qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] 748053: qapi: Use generated TestStruct machin


From: GitHub
Subject: [Qemu-commits] [qemu/qemu] 748053: qapi: Use generated TestStruct machinery in tests
Date: Tue, 10 Nov 2015 03:00:08 -0800

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 748053c97b11039f657525fd7d57a39806d8083e
      
https://github.com/qemu/qemu/commit/748053c97b11039f657525fd7d57a39806d8083e
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M tests/qapi-schema/qapi-schema-test.json
    M tests/qapi-schema/qapi-schema-test.out
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c
    M tests/test-qmp-output-visitor.c
    M tests/test-visitor-serialization.c

  Log Message:
  -----------
  qapi: Use generated TestStruct machinery in tests

Commit d88f5fd and friends first introduced the various test-qmp-*
tests in 2011, with duplicated hand-rolled TestStruct machinery,
to make sure the qapi visitor interface was tested.  Later, commit
4f193e3 in 2013 added a .json file for further testing use by the
files, but without consolidating any of the existing hand-rolled
visitors.  And with four copies, subtle differences have crept in,
between the tests themselves (mainly whitespace differences, but
also a question of whether to use NULL or "TestStruct" when
calling visit_start_struct()) and from what the generator produces
(the hand-rolled versions did not cater to partially-allocated
objects, because they did not have a deallocation usage).

Of course, just because the visitor interface is tested does not
mean it is a sane interface; and future patches will be changing
some of the visitor contracts.  Rather than having to duplicate
the cleanup work in each copy of the TestStruct visitor, and keep
each hand-rolled copy in sync with what the generator supplies, we
might as well just test what the generator should give us in the
first place.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: bd20588d19e9ff0e94b2d4ca3b5d6b3b3d6a1274
      
https://github.com/qemu/qemu/commit/bd20588d19e9ff0e94b2d4ca3b5d6b3b3d6a1274
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M tests/test-qmp-output-visitor.c

  Log Message:
  -----------
  qapi: Strengthen test of TestStructList

Make each list element different, to ensure that order is
preserved, and use the generated free function instead of
hand-rolling our own to ensure (under valgrind) that the
list is properly cleaned.

Suggested-by: Markus Armbruster <address@hidden>
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: cc9f60d4a2a4bf2578a9309a18f1c4602c9f5ce7
      
https://github.com/qemu/qemu/commit/cc9f60d4a2a4bf2578a9309a18f1c4602c9f5ce7
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M include/qapi/qmp/qobject.h

  Log Message:
  -----------
  qobject: Protect against use-after-free in qobject_decref()

Adding an assertion to qobject_decref() will ensure that a
programming error causing use-after-free will result in
immediate failure (provided no other thread has started
using the memory) instead of silently attempting to wrap
refcnt around and leaving the problem to potentially bite
later at a harder point to diagnose.

Suggested-by: Markus Armbruster <address@hidden>
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 0920a17199d23b3def3a60fa1fbbdeadcdda452d
      
https://github.com/qemu/qemu/commit/0920a17199d23b3def3a60fa1fbbdeadcdda452d
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c

  Log Message:
  -----------
  qapi: Share test_init code in test-qmp-input*

Rather than duplicate the body of two functions just to
decide between qobject_from_jsonv() and qobject_from_json(),
exploit the fact that qobject_from_jsonv() intentionally
takes 'va_list *' instead of the more common 'va_list', and
that qobject_from_json() just calls qobject_from_jsonv(,NULL).
For each file, our two existing init functions then become
thin wrappers around a new internal function, and future
updates to initialization don't have to be duplicated.

Suggested-by: Markus Armbruster <address@hidden>
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
[Two old comment typos fixed]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: b18f1141d0afa00de11a8e079f4f5305c9e36893
      
https://github.com/qemu/qemu/commit/b18f1141d0afa00de11a8e079f4f5305c9e36893
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c
    M tests/test-qmp-output-visitor.c

  Log Message:
  -----------
  qapi: Plug leaks in test-qmp-*

Make valgrind happy with the current state of the tests, so that
it is easier to see if future patches introduce new memory problems
without being drowned in noise.  Many of the leaks were due to
calling a second init without tearing down the data from an earlier
visit.  But since teardown is already idempotent, and we already
register teardown as part of input_visitor_test_add(), it is nicer
to just make init() safe to call multiple times than it is to have
to make all tests call teardown.

Another common leak was forgetting to clean up an error object,
after testing that an error was raised.

Another leak was in test_visitor_in_struct_nested(), failing to
clean the base member of UserDefTwo.  Cleaning that up left
check_and_free_str() as dead code (since using the qapi_free_*
takes care of recursion, and we don't want double frees).

A final leak was in test_visitor_out_any(), which was reassigning
the qobj local variable to a subset of the overall structure
needing freeing; it did not result in a use-after-free, but
was not cleaning up all the qdict.

test-qmp-event and test-qmp-commands were already clean.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 3f66f764ee25f10d3e1144ebc057a949421b7728
      
https://github.com/qemu/qemu/commit/3f66f764ee25f10d3e1144ebc057a949421b7728
  Author: Eric Blake <address@hidden>
  Date:   2015-11-09 (Mon, 09 Nov 2015)

  Changed paths:
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c
    M tests/test-qmp-output-visitor.c
    M tests/test-visitor-serialization.c

  Log Message:
  -----------
  qapi: Simplify non-error testing in test-qmp-*

By using &error_abort, we can avoid a local err variable in
situations where we expect success.  It also has the nice
effect that if the test breaks, the error message from
error_abort tends to be nicer than that of g_assert().

This patch has an additional bonus of fixing several call sites that
were passing &err to two different functions without checking it in
between.  In general that is unsafe practice; because if the first
function sets an error, the second function could abort() if it tries to
set a different error. We got away with it because we were asserting
that err was NULL through the entire chain, but switching to
&error_abort avoids the questionable practice up front.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: a12a5a1a0132527afe87c079e4aae4aad372bd94
      
https://github.com/qemu/qemu/commit/a12a5a1a0132527afe87c079e4aae4aad372bd94
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M include/qapi/error.h
    M tests/test-qmp-commands.c
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c
    M util/error.c

  Log Message:
  -----------
  qapi: Simplify error cleanup in test-qmp-*

We have several tests that perform multiple sub-actions that are
expected to fail.  Asserting that an error occurred, then clearing
it up to prepare for the next action, turned into enough
boilerplate that it was sometimes forgotten (for example, a number
of tests added to test-qmp-input-visitor.c in d88f5fd leaked err).
Worse, if an error is not reset to NULL, we risk invalidating
later use of that error (passing a non-NULL err into a function
is generally a bad idea).  Encapsulate the boilerplate into a
single helper function error_free_or_abort(), and consistently
use it.

The new function is added into error.c for use everywhere,
although it is anticipated that testsuites will be the main
client.

Signed-off-by: Eric Blake <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 12fafd7cedad51854c468ea0496a6542b3222b29
      
https://github.com/qemu/qemu/commit/12fafd7cedad51854c468ea0496a6542b3222b29
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M tests/test-qmp-output-visitor.c

  Log Message:
  -----------
  qapi: More tests of alternate output

The testsuite was only covering that we could output the 'int'
branch of an alternate (no additional allocation/cleanup required).
Add a test of the 'str' branch, to make sure that things still
work even when a branch involves allocation.

Update to modern style of g_new0() over g_malloc0() while
touching it.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: dd5ee2c2d3e3a17647ddd9bfa97935b8cb5dfa40
      
https://github.com/qemu/qemu/commit/dd5ee2c2d3e3a17647ddd9bfa97935b8cb5dfa40
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M scripts/qapi-visit.py
    M tests/test-qmp-input-visitor.c

  Log Message:
  -----------
  qapi: Test failure in middle of array parse

Our generated list visitors have the same problem as has been
mentioned elsewhere (see commit 2f52e20): they allocate data
even on failure. An upcoming patch will correct things to
provide saner guarantees, but first we need to expose the
behavior in the testsuite to ensure we aren't introducing any
memory usage bugs.

There are more test cases throughout the test-qmp-input-* tests
that already deal with partial allocation; a later commit will
clean up all visit_type_FOO(), without marking all of the tests
with FIXME at this time.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 2533377c7b0c686d1510ed6499cedf938607e805
      
https://github.com/qemu/qemu/commit/2533377c7b0c686d1510ed6499cedf938607e805
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M tests/test-qmp-input-visitor.c

  Log Message:
  -----------
  qapi: More tests of input arrays

Our testsuite had no coverage of empty arrays, nor of what
happens when the input does not match the expected type.
Useful to have, especially if we start changing the visitor
contracts.

I did not think it worth duplicating these additions to
test-qmp-input-strict; since all strict mode does is add
the ability to reject JSON input that has more keys than
what the visitor expects, yet the additions in this patch
error out earlier than that point regardless of whether
strict mode was requested.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: ce5fcb472d512455a8d13fae4c04ecf8eb00573b
      
https://github.com/qemu/qemu/commit/ce5fcb472d512455a8d13fae4c04ecf8eb00573b
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M docs/qapi-code-gen.txt
    M scripts/qapi-introspect.py

  Log Message:
  -----------
  qapi: Provide nicer array names in introspection

For the sake of humans reading introspection output, it is nice
to have the name of implicit array types be recognizable as
arrays of the underlying type.  However, while this patch allows
humans to skip from a command with return type "[123]" straight
to the definition of type "123" without having to first inspect
type "[123]", document that this shortcut should not be taken by
client apps.

This makes the resulting introspection string slightly larger by
default (just over 200 bytes), but it's in the noise (less than
0.3% of the overall 70k size of 'query-qmp-capabilities').

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: f5455044201747fd72531f5e8c1b1e9c56573d9c
      
https://github.com/qemu/qemu/commit/f5455044201747fd72531f5e8c1b1e9c56573d9c
  Author: Eric Blake <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M docs/qapi-code-gen.txt
    M qapi/introspect.json

  Log Message:
  -----------
  qapi-introspect: Document lack of sorting

qapi-code-gen.txt already claims that types, commands, and
events share a common namespace; set this in stone by further
documenting that our introspection output will never have
collisions with the same name tied to more than one meta-type.

Our largest QMP enum currently has 125 values, our largest
object type has 27 members, and the mean for each is less than
10.  These sizes are small enough that the per-element overhead
of O(log n) binary searching probably outweighs the speed
possible with direct O(n) linear searching (a better algorithm
with more overhead will only beat a leaner naive algorithm only
as you scale to larger input sizes).

Arguably, the overall SchemaInfo array could be sorted by name;
there, we currently have 531 entities, large enough for a binary
search to be faster than linear.  However, remember that we have
mutually-recursive types, which means there is no topological
ordering that will allow clients to learn all information about
that type in a single linear pass; thus clients will want to do
random access over the data, and they will probably read the
introspection output into a hashtable for O(1) lookup rather
than O(log n) binary searching, at which point, pre-sorting our
introspection output doesn't help the client.

It doesn't help that sorting can be subjective if you introduce
locales into the mix (I'm not experienced enough with Python
to know for sure, but at least it looks like it defaults to
sorting in the C locale even when run under a different locale).
And while our current introspection output is deterministic
(because we visit entities in a sorted order), we may want
to change that order in the future (such as using OrderedDict
to stick to .json declaration order).

For these reasons, we simply document that clients should not
rely on any particular order of items in introspection output.
And since it is now a documented part of the contract, we have
the freedom to later rearrange output if needed, without
worrying about breaking well-written clients.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: a8b4f9585a0bf5186fca793ce2c5d754cd8ec49a
      
https://github.com/qemu/qemu/commit/a8b4f9585a0bf5186fca793ce2c5d754cd8ec49a
  Author: Peter Maydell <address@hidden>
  Date:   2015-11-10 (Tue, 10 Nov 2015)

  Changed paths:
    M docs/qapi-code-gen.txt
    M include/qapi/error.h
    M include/qapi/qmp/qobject.h
    M qapi/introspect.json
    M scripts/qapi-introspect.py
    M scripts/qapi-visit.py
    M tests/qapi-schema/qapi-schema-test.json
    M tests/qapi-schema/qapi-schema-test.out
    M tests/test-qmp-commands.c
    M tests/test-qmp-input-strict.c
    M tests/test-qmp-input-visitor.c
    M tests/test-qmp-output-visitor.c
    M tests/test-visitor-serialization.c
    M util/error.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-11-10' into 
staging

QAPI patches

# gpg: Signature made Tue 10 Nov 2015 07:12:25 GMT using RSA key ID EB918653
# gpg: Good signature from "Markus Armbruster <address@hidden>"
# gpg:                 aka "Markus Armbruster <address@hidden>"

* remotes/armbru/tags/pull-qapi-2015-11-10:
  qapi-introspect: Document lack of sorting
  qapi: Provide nicer array names in introspection
  qapi: More tests of input arrays
  qapi: Test failure in middle of array parse
  qapi: More tests of alternate output
  qapi: Simplify error cleanup in test-qmp-*
  qapi: Simplify non-error testing in test-qmp-*
  qapi: Plug leaks in test-qmp-*
  qapi: Share test_init code in test-qmp-input*
  qobject: Protect against use-after-free in qobject_decref()
  qapi: Strengthen test of TestStructList
  qapi: Use generated TestStruct machinery in tests

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/ce278618b088...a8b4f9585a0b

reply via email to

[Prev in Thread] Current Thread [Next in Thread]