Commit Graph

39 Commits

Author SHA1 Message Date
Eric Blake
d00341af38 qapi: Fix alternates that accept 'number' but not 'int'
The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT.  However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' and some other type, but not 'int', would reject
integral values.

With this patch, we now have the following desirable table:

    alternate has      case selected for
    'int'  'number'    QTYPE_QINT  QTYPE_QFLOAT
      no        no     error       error
      no       yes     'number'    'number'
     yes        no     'int'       error
     yes       yes     'int'       'number'

While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-8-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17 08:21:28 +01:00
Eric Blake
0426d53c65 qapi: Simplify visiting of alternate types
Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.

This has a couple of subtle bugs.  First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS.

Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.

Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type().  Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).

However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it).  A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.

This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter.  This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names).  Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.

Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types.  I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
  typedef enum FooKind {
      FOO_KIND_A = QTYPE_QDICT,
      FOO_KIND_B = QTYPE_QINT,
  } FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.

There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
  {"execute":"blockdev-add", "arguments":{"options":
    {"driver":"raw", "id":"a", "file":true}}}
failed with:
  {"error": {"class": "GenericError",
    "desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work).  Now it fails with:
  {"error": {"class": "GenericError",
    "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17 08:21:28 +01:00
Eric Blake
2533377c7b 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 <eblake@redhat.com>
Message-Id: <1446791754-23823-11-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-10 08:09:14 +01:00
Eric Blake
dd5ee2c2d3 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 <eblake@redhat.com>
Message-Id: <1446791754-23823-10-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-10 08:09:14 +01:00
Eric Blake
a12a5a1a01 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 <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-10 08:08:21 +01:00
Eric Blake
3f66f764ee 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 <eblake@redhat.com>
Message-Id: <1446791754-23823-7-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-09 16:45:05 +01:00
Eric Blake
b18f1141d0 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 <eblake@redhat.com>
Message-Id: <1446791754-23823-6-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-09 16:45:05 +01:00
Eric Blake
0920a17199 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 <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-5-git-send-email-eblake@redhat.com>
[Two old comment typos fixed]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-09 16:45:05 +01:00
Eric Blake
748053c97b 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 <eblake@redhat.com>
Message-Id: <1446791754-23823-2-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-09 16:45:05 +01:00
Eric Blake
c363acef77 tests: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a non-variant
member's name.

Make the conversion to the new layout for testsuite code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-15-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-02 08:30:27 +01:00
Eric Blake
ddf2190896 qapi: Unbox base members
Rather than storing a base class as a pointer to a box, just
store the fields of that base class in the same order, so that
a child struct can be directly cast to its parent.  This gives
less malloc overhead, less pointer dereferencing, and even less
generated code.  Compare to the earlier commit 1e6c1616a "qapi:
Generate a nicer struct for flat unions" (although that patch
had fewer places to change, as less of qemu was directly using
qapi structs for flat unions).  It also allows us to turn on
automatic type-safe wrappers for upcasting to the base class
of a struct.

Changes to the generated code look like this in qapi-types.h:

| struct SpiceChannel {
|-    SpiceBasicInfo *base;
|+    /* Members inherited from SpiceBasicInfo: */
|+    char *host;
|+    char *port;
|+    NetworkAddressFamily family;
|+    /* Own members: */
|     int64_t connection_id;

as well as additional upcast functions like qapi_SpiceChannel_base().
Meanwhile, changes to qapi-visit.c look like:

| static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp)
| {
|     Error *err = NULL;
|
|-    visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
|+    visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err);
|     if (err) {

(the cast is necessary, since our upcast wrappers only deal with a
single pointer, not pointer-to-pointer); plus the wholesale
elimination of some now-unused visit_type_implicit_FOO() functions.

Without boxing, the corner case of one empty struct having
another empty struct as its base type now requires inserting a
dummy member (previously, the 'Base *base' member sufficed).

And now that we no longer consume a 'base' member in the generated
C struct, we can delete the former negative struct-base-clash-base
test.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-11-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-02 08:30:26 +01:00
Eric Blake
30594fe1cd qapi: Prefer typesafe upcasts to qapi base classes
A previous patch (commit 1e6c1616) made it possible to
directly cast from a qapi flat union type to its base type.
However, it requires the use of a C cast, which turns off
compiler type-safety checks.  Fortunately, no such casts
exist, just yet.

Regardless, add inline type-safe wrappers named
qapi_FOO_base() for any union type FOO that has a base,
which can be used for a safer upcast, and enhance the
testsuite to cover the new functionality.

A future patch will extend the upcast support to structs,
where such conversions do exist already.

Note that C makes const-correct upcasts annoying because
it lacks overloads; these functions cast away const so that
they can accept user pointers whether const or not, and the
result in turn can be assigned to normal or const pointers.
Alternatively, this could have been done with macros, but
type-safe macros are hairy, and not worthwhile here.

This patch just adds upcasts.  None of our code needed to
downcast from a base qapi class to a child.  Also, in the
case of grandchildren (such as BlockdevOptionsQcow2), the
caller will need to call two functions to get to the inner
base (although it wouldn't be too hard to generate a
qapi_FOO_base_base() if desired).  If a user changes qapi
to alter the base class hierarchy, such as going from
'A -> C' to 'A -> B -> C', it will change the type of
'qapi_C_base()', and the compiler will point out the places
that are affected by the new base.

One alternative was proposed, but was deemed too ugly to use
in practice: the generators could output redundant
information using anonymous types:
| struct Child {
|     union {
|         struct {
|             Type1 parent_member1;
|             Type2 parent_member2;
|         };
|         Parent base;
|     };
| };
With that ugly proposal, for a given qapi type, obj->member
and obj->base.member would refer to the same storage; allowing
convenience in working with members without needing 'base.'
allowing typesafe upcast without needing a C cast by accessing
'&obj->base', and allowing downcasts from the parent back to
the child possible through container_of(obj, Child, base).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-10-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-02 08:30:26 +01:00
Eric Blake
9c51b44129 qapi: Test use of 'number' within alternates
Add some testsuite exposure for use of a 'number' as part of
an alternate.  The current state of the tree has a few bugs
exposed by this: our input parser depends on the ordering of
how the qapi schema declared the alternate, and the parser
does not accept integers for a 'number' in an alternate even
though it does for numbers outside of an alternate.

Mixing 'int' and 'number' in the same alternate is unusual,
since both are supplied by json-numbers, but there does not
seem to be a technical reason to forbid it given that our
json lexer distinguishes between json-numbers that can be
represented as an int vs. those that cannot.

Improve the existing test_visitor_in_alternate() to match the
style of the new test_visitor_in_alternate_number(), and to
ensure full coverage of all possible qtype parsing.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1443565276-4535-9-git-send-email-eblake@redhat.com>
[Eric's follow-up fixes squashed in]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-10-12 18:46:13 +02:00
Markus Armbruster
28770e057f qapi: Introduce a first class 'any' type
It's first class, because unlike '**', it actually works, i.e. doesn't
require 'gen': false.

'**' will go away next.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
2015-09-21 09:56:49 +02:00
Markus Armbruster
441cbac0c7 qapi-visit: Convert to QAPISchemaVisitor, fixing bugs
Fixes flat unions to visit the base's base members (the previous
commit merely added them to the struct).  Same test case.

Patch's effect on visit_type_UserDefFlatUnion():

     static void visit_type_UserDefFlatUnion_fields(Visitor *m, UserDefFlatUnion **obj, Error **errp)
     {
         Error *err = NULL;

    +    visit_type_int(m, &(*obj)->integer, "integer", &err);
    +    if (err) {
    +        goto out;
    +    }
         visit_type_str(m, &(*obj)->string, "string", &err);
         if (err) {
             goto out;

Test cases updated for the bug fix.

Fixes alternates to generate a visitor for their implicit enumeration
type.  None of them are currently used, obviously.  Example:
block-core.json's BlockdevRef now generates
visit_type_BlockdevRefKind().

Code is generated in a different order now, and therefore has got a
few new forward declarations.  Doesn't matter.

The guard QAPI_VISIT_BUILTIN_VISITOR_DECL is renamed to
QAPI_VISIT_BUILTIN.

The previous commit's two ugly special cases exist here, too.  Mark
both TODO.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
2015-09-21 09:56:40 +02:00
Eric Blake
2f52e20597 qapi: Document that input visitor semantics are prone to leaks
Most functions that can return a pointer or set an Error ** value
are decent enough to guarantee a NULL return when reporting an error.
Not so with our generated qapi visitor functions.  If the caller
is not careful to clean up partially-allocated objects on error,
then the caller suffers a memory leak.

Properly fixing it is probably complex enough to save for a later
day, so merely document it for now.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1438295587-19069-1-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-09-04 15:47:14 +02:00
Markus Armbruster
0f61af3eb3 qapi: Fix generated code when flat union has member 'kind'
A flat union's tag member gets renamed to 'kind' in the generated
code.  Breaks when another member named 'kind' exists.

Example, adapted from qapi-schema-test.json:

    { 'struct': 'UserDefUnionBase',
      'data': { 'kind': 'str', 'enum1': 'EnumOne' } }

We generate:

    struct UserDefFlatUnion
    {
        EnumOne kind;
        union {
            void *data;
            UserDefA *value1;
            UserDefB *value2;
            UserDefB *value3;
        };
        char *kind;
    };

Kill the silly rename.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
2015-09-04 15:47:13 +02:00
Eric Blake
6446a59276 qapi: Drop tests for inline nested structs
A future patch will be using a 'name':{dictionary} entry in the
QAPI schema to specify a default value for an optional argument;
but existing use of inline nested structs conflicts with that goal.

More precisely, a definition in the QAPI schema associates a name
with a set of properties:

Example 1: { 'struct': 'Foo', 'data': { MEMBERS... } }
associates the global name 'Foo' with properties (meta-type struct)
and MEMBERS...

Example 2: 'mumble': TYPE
within MEMBERS... above associates 'mumble' with properties (type
TYPE) and (optional false) within type Foo

The syntax of example 1 is extensible; if we need another property,
we add another name/value pair to the dictionary (such as
'base':TYPE).  The syntax of example 2 is not extensible, because
the right hand side can only be a type.

We have used name encoding to add a property: "'*mumble': 'int'"
associates 'mumble' with (type int) and (optional true).  Nice,
but doesn't scale.  So the solution is to change our existing uses
to be syntactic sugar to an extensible form:

   NAME: TYPE   --> NAME:  { 'type': TYPE, 'optional': false }
   *ONAME: TYPE --> ONAME: { 'type': TYPE, 'optional': true }

This patch fixes the testsuite to avoid inline nested types, by
breaking the nesting into explicit types; it means that the type
is now boxed instead of unboxed in C code, but makes no difference
on the wire (and if desired, a later patch could change the
generator to not do so much boxing in C).  When touching code to
add new allocations, also convert existing allocations to
consistently prefer typesafe g_new0 over g_malloc0 when a type
name is involved.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-05-05 18:39:02 +02:00
Eric Blake
b6fcf32d9b qapi: Merge UserDefTwo and UserDefNested in tests
In the testsuite, UserDefTwo and UserDefNested were identical
structs other than the member names.  Reduce code duplication by
having just one type, and choose names that also favor reuse.
This will also make it easier for a later patch to get rid of
inline nested types in QAPI.  When touching code related to
allocations, convert g_malloc0(sizeof(Type)) to the more typesafe
g_new0(Type, 1).

Ensure that 'make check-qapi-schema check-unit' still passes.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-05-05 18:39:02 +02:00
Eric Blake
ab04526744 qapi: Rename anonymous union type in test
Reduce churn in the future patch that replaces anonymous unions
with a new metatype 'alternate' by changing 'AnonUnion' to
'Alternate'.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-05-05 18:39:00 +02:00
Eric Blake
a8d4a2e4d7 qapi: Forbid base without discriminator in unions
None of the existing QMP or QGA interfaces uses a union with a
base type but no discriminator; it is easier to avoid this in the
generator to save room for other future extensions more likely to
be useful.  An earlier commit added a union-base-no-discriminator
test to ensure that we eventually give a decent error message;
likewise, removing UserDefUnion outright is okay, because we moved
all the tests we wish to keep into the tests of the simple union
UserDefNativeListUnion in the previous commit.  Now is the time to
actually forbid simple union with base, and remove the last
vestiges from the testsuite.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-05-05 18:39:00 +02:00
Eric Blake
805017b779 qapi: Clean up test coverage of simple unions
The tests of UserDefNativeListUnion serve to validate code
generation of simple unions without a base type, except that it
did not have full coverage in the strict test.  The next commits
will remove tests and support for simple unions with a base type,
so there is no real loss at repurposing that test here as
opposed to churn of adding a new test then deleting the old one.

Fix some indentation and long lines while at it.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-05-05 18:39:00 +02:00
Markus Armbruster
297a3646c2 qapi: Replace uncommon use of the error API by the common one
We commonly use the error API like this:

    err = NULL;
    foo(..., &err);
    if (err) {
        goto out;
    }
    bar(..., &err);

Every error source is checked separately.  The second function is only
called when the first one succeeds.  Both functions are free to pass
their argument to error_set().  Because error_set() asserts no error
has been set, this effectively means they must not be called with an
error set.

The qapi-generated code uses the error API differently:

    // *errp was initialized to NULL somewhere up the call chain
    frob(..., errp);
    gnat(..., errp);

Errors accumulate in *errp: first error wins, subsequent errors get
dropped.  To make this work, the second function does nothing when
called with an error set.  Requires non-null errp, or else the second
function can't see the first one fail.

This usage has also bled into visitor tests, and two device model
object property getters rtc_get_date() and balloon_stats_get_all().

With the "accumulate" technique, you need fewer error checks in
callers, and buy that with an error check in every callee.  Can be
nice.

However, mixing the two techniques is confusing.  You can't use the
"accumulate" technique with functions designed for the "check
separately" technique.  You can use the "check separately" technique
with functions designed for the "accumulate" technique, but then
error_set() can't catch you setting an error more than once.

Standardize on the "check separately" technique for now, because it's
overwhelmingly prevalent.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-15 14:00:46 -04:00
Markus Armbruster
196857f8bf tests/qapi-schema: Drop superfluous error_is_set()
visit_type_TestStruct() does nothing when called with an error set.
Callers shouldn't do that, and no caller does.  Drop the superfluous
test.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-09 09:11:31 -04:00
Markus Armbruster
e940f543ae qmp hmp: Consistently name Error * objects err, and not errp
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-08 14:20:00 -04:00
Wenchao Xia
5223070c47 qapi script: do not allow string discriminator
Since enum based discriminators provide better type-safety and
ensure that future qapi additions do not forget to adjust dependent
unions, forbid using string as discriminator from now on.

Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-03-11 09:07:42 -04:00
Markus Armbruster
2fc0043283 tests/qapi-schema: Cover flat union types
The test demonstrates a generator bug: the generated struct
UserDefFlatUnion doesn't include members for the indirect base
UserDefZero.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-03-03 11:16:45 -05:00
Markus Armbruster
7ad993b480 tests/qapi-schema: Cover union types with base
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-03-03 11:16:45 -05:00
Markus Armbruster
aabbd472a0 tests/qapi-schema: Cover complex types with base
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-03-03 11:16:45 -05:00
Markus Armbruster
2c38b60010 tests/qapi-schema: Cover anonymous union types
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-03-03 11:16:45 -05:00
Markus Armbruster
84d18f065f Use error_is_set() only when necessary
error_is_set(&var) is the same as var != NULL, but it takes
whole-program analysis to figure that out.  Unnecessarily hard for
optimizers, static checkers, and human readers.  Dumb it down to
obvious.

Gets rid of several dozen Coverity false positives.

Note that the obvious form is already used in many places.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-02-17 11:57:23 -05:00
Wenchao Xia
8aa15b6e52 tests: fix memleak in error path test for input visitor
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 1383676551-18806-3-git-send-email-xiawenc@linux.vnet.ibm.com
Cc: qemu-stable@nongnu.org
Signed-off-by: Anthony Liguori <aliguori@amazon.com>
2013-11-05 19:58:38 -08:00
Stefan Weil
dfc6f86567 misc: Use g_assert_not_reached for code which is expected to be unreachable
The macro g_assert_not_reached is a better self documenting replacement
for assert(0) or assert(false).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
2013-07-27 11:22:54 +04:00
Michael Roth
199e0f17f2 qapi: add native list coverage for QMP input visitor tests
This exercises schema-generated visitors for native list types and does
some sanity checking on validity of deserialized data.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Amos Kong <akong@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2013-05-23 09:44:21 -04:00
Michael Roth
e92cfa0d90 qapi: add QMP input test for large integers
Large integers previously got capped to LLONG_MAX/LLONG_MIN so we could
store them as int64_t. This could lead to silent errors occuring.

Now, we use a double to handle these cases.

Add a test to confirm that QMPInputVisitor handles this as expected if
we're expected an integer value: errors for out of range integer values
that got promoted to doubles in this fashion.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Amos Kong <akong@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2013-05-23 09:44:21 -04:00
Paolo Bonzini
7b1b5d1913 qapi: move include files to include/qobject/
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2012-12-19 08:31:31 +01:00
Paolo Bonzini
79ee7df885 qapi: move inclusions of qemu-common.h from headers to .c files
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2012-12-19 08:31:30 +01:00
Paolo Bonzini
d195325b05 qapi: fix error propagation
Don't overwrite / leak previously set errors.
Make traversal cope with missing mandatory sub-structs.
Don't try to end a container that could not be started.

v1->v2:
- unchanged

v2->v3:
- instead of examining, assert that we never overwrite errors with
  error_set()
- allow visitors to set a NULL struct pointer successfully, so traversal
  of incomplete objects can continue
- check for a NULL "obj" before accessing "(*obj)->has_XXX" (this is not a
  typo, "obj != NULL" implies "*obj != NULL" here)
- fix start_struct / end_struct balance for unions as well

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
2012-07-23 11:55:17 +01:00
Paolo Bonzini
b93b63f574 test makefile overhaul
This introduces new test reporting infrastructure based on
gtester and gtester-report.

Also, all existing tests are moved to tests/, and tests/Makefile
is reorganized to factor out the commonalities in the rules.

Signed-off-by: Anthony Liguori <aliguori@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
2012-03-30 08:14:11 -05:00