In some cases an input visitor might bail out on filling out a
struct for various reasons, such as missing fields when running
in strict mode. In the case of a QAPI Union type, this may lead
to cases where the .kind field which encodes the union type
is uninitialized. Subsequently, other visitors, such as the
dealloc visitor, may use this .kind value as if it were
initialized, leading to assumptions about the union type which
in this case may lead to segfaults. For example, freeing an
integer value.
However, we can generally rely on the fact that the always-present
.data void * field that we generate for these union types will
always be NULL in cases where .kind is uninitialized (at least,
there shouldn't be a reason where we'd do this purposefully).
So pass this information on to Visitor implementation via these
optional start_union/end_union interfaces so this information
can be used to guard against the situation above. We will make
use of this information in a subsequent patch for the dealloc
visitor.
Cc: qemu-stable@nongnu.org
Reported-by: Fam Zheng <famz@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 1408557576-14574-3-git-send-email-mreitz@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
A space after * when declaring a pointer type is redundant.
Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
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>
In preparation of error handling changes. Bonus: generates less
duplicated code.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
generate_visit_struct_fields() generates the base type's struct member
name both with and without the field prefix. Harmless, because the
field prefix is always empty there: only unboxed complex members have
a prefix, and those can't have a base type.
Clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
By un-inlining the visit of nested complex types.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Changing implicit indentation in the middle of generating a block
makes following the code being generated unnecessarily hard.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Semantics of end_optional() differ subtly from the other end_FOO()
callbacks: when start_FOO() succeeds, the matching end_FOO() gets
called regardless of what happens in between. end_optional() gets
called only when everything in between succeeds as well. Entirely
undocumented, like all of the visitor API.
The only user of Visitor Callback end_optional() never did anything,
and was removed in commit 9f9ab46.
I'm about to clean up error handling in the generated visitor code,
and end_optional() is in my way. No users mean no test cases, and
making non-trivial cleanup transformations without test cases doesn't
strike me as a good idea.
Drop end_optional(), and rename start_optional() to optional(). We
can always go back to a pair of callbacks when we have an actual need.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Use an explicit input file on the command-line instead of reading from standard
input.
It also outputs the proper file name when there's an error.
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
By default, any union will automatically generate a enum type as
"[UnionName]Kind" in C code, and it is duplicated when the discriminator
is specified as a pre-defined enum type in schema. After this patch,
the pre-defined enum type will be really used as the switch case
condition in generated C code, if discriminator is an enum field.
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Prior to this patch, qapi-visit.py used custom code to generate enum
names used for handling a qapi union. Fix it to instead reuse common
code, with identical generated results, and allowing future updates to
generation to only need to touch one place.
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>
Visitors get passed a pointer to the visited object. The generated
visitors try to cope with this pointer being null in some places, for
instance like this:
visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
visit_start_optional() passes its second argument to Visitor method
start_optional. Three out of three methods dereference it
unconditionally.
I fail to see how this pointer could legitimately be null.
All this useless null checking is highly redundant, which Coverity
duly reports. About 200 times.
Remove the useless null checks.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
The scripts carry this copyright notice:
# This work is licensed under the terms of the GNU GPLv2.
# See the COPYING.LIB file in the top-level directory.
The sentences contradict each other, as COPYING.LIB contains the LGPL
2.1. Michael Roth says this was a simple pasto, and he meant to refer
COPYING. Let's fix that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
We say we support python 2.4, but python 2.4.3 does not
support the "expr if test else expr" syntax used here.
This allows QEMU to compile on RHEL 5.3, the last release for ia64.
Signed-off-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
This introduces a new 'base' key for struct definitions that refers to
another struct type. On the JSON level, the fields of the base type are
included directly into the same namespace as the fields of the defined
type, like with unions. On the C level, a pointer to a struct of the
base type is included.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The discriminator for anonymous unions is the data type. This allows to
have a union type that allows both of these:
{ 'file': 'my_existing_block_device_id' }
{ 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }
Unions like this are specified in the schema with an empty dict as
discriminator. For this example you could take:
{ 'union': 'BlockRef',
'discriminator': {},
'data': { 'definition': 'BlockOptions',
'reference': 'str' } }
{ 'type': 'ExampleObject',
'data: { 'file': 'BlockRef' } }
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Instead of the rather verbose syntax that distinguishes base and
subclass fields...
{ "type": "file",
"read-only": true,
"data": {
"filename": "test"
} }
...we can now have both in the same namespace, allowing a more direct
mapping of the command line, and moving fields between the common base
and subclasses without breaking the API:
{ "driver": "file",
"read-only": true,
"filename": "test" }
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Teach visitor generators about native types so they can generate the
appropriate visitor routines.
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>
Currently we assume non-list types when generating visitor routines for
union types. This is broken, since values like ['Type'] need to mapped
to 'TypeList'.
We already have a type_name() function to handle this that we use for
generating struct visitors, so use that here as well.
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>
Enum values are always preceded by the uppercase name of the enum, so
they do not conflict with reserved words.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Report from smatch:
qapi-visit.c:1640 visit_type_BlockdevAction(8) error:
we previously assumed 'obj' could be null (see line 1639)
qapi-visit.c:2432 visit_type_NetClientOptions(8) error:
we previously assumed 'obj' could be null (see line 2431)
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Currently, if we define an 'enum' and use it in one command's
data, list struct for enum could not be generated, but it's
used in qmp function.
For example: KeyCodesList could not be generated.
>>> qapi-schema.json:
{ 'enum': 'KeyCodes',
'data': [ 'shift', 'alt' ... ] }
{ 'command': 'sendkey',
'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
>>> qmp-command.h:
void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
hold_time, Error **errp);
This patch lets qapi generate list struct and visit_list for enum.
Signed-off-by: Amos Kong <akong@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
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>
Right now, the semantics of next_list are complicated. The caller must:
* call start_list
* call next_list for each element *including the first*
* on the first call to next_list, the second argument should point to
NULL and the result is the head of the list. On subsequent calls,
the second argument should point to the last node (last result of
next_list) and next_list itself tacks the element at the tail of the
list.
This works for both input and output visitor, but having the visitor
write memory when it is only reading the list is ugly. Plus, relying
on *list to detect the first call is tricky and undocumented.
We can initialize so->entry in next_list instead of start_list, leaving
it NULL in start_list. This way next_list sees clearly whether it is
on the first call---as a bonus, it discriminates the cases based on
internal state of the visitor rather than external state. We can
also pull the assignment of the list head from generated code up to
next_list.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Objects going through the dealloc visitor can be only partially allocated.
Detect the situation and avoid a segfault. This also helps with the
input visitor, when there are errors.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
We can exit very soon if we enter a visitor with a preexisting error.
This simplifies some cases because we will not have to deal with
obj being non-NULL while *obj is NULL.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
The fixes to qapi code generation had multiple bugs:
- the Null class used to drop output was missing some methods
- in some scripts it was never instantiated, leading to a None return,
which is missing even more methods
- the --source and --header options were swapped
Luckily, all those bugs were hidden by a makefile bug which caused the
old behaviour (with the race) to be invoked.
Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Make's multiple output syntax
x.c x.h: x.template
gen < x.template
actually invokes the command once for x.c and once for x.h (with differing $@
in each invocation). During a parallel build, the two commands may be invoked
in parallel; this opens up a race, where the second invocation trashes a file
supposedly produced during the first, and now in use by a dependent command.
The various qapi code generators are susceptible to this; fix by making them
generate just one file per invocation.
Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Modify logic such that we never assign values to the list head argument
to progress through the list on subsequent iterations, instead rely only
on having our return value passed back in as an argument on the next
call. Also update QMP I/O visitors and test cases accordingly, and add a
missing test case for QmpOutputVisitor.
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
This is the code generator for qapi visiter functions used to
marshal/unmarshal/dealloc qapi types. It generates the following 2
files:
$(prefix)qapi-visit.c: visiter function for a particular c type, used
to automagically convert qobjects into the
corresponding C type and vice-versa, and well
as for deallocation memory for an existing C
type
$(prefix)qapi-visit.h: declarations for previously mentioned visiter
functions
$(prefix) is used as decribed for qapi-types.py
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@gmail.com>
