There's no reason to do two malloc's for an alternate type visiting
a QAPI struct; let's just inline the struct directly as the C union
branch of the struct.
Surprisingly, no clients were actually using the struct member prior
to this patch outside of the testsuite; an earlier patch in the series
added some testsuite coverage to make the effect of this patch more
obvious.
In qapi.py, c_type() gains a new is_unboxed flag to control when we
are emitting a C struct unboxed within the context of an outer
struct (different from our other two modes of usage with no flags
for normal local variable declarations, and with is_param for adding
'const' in a parameter list). I don't know if there is any more
pythonic way of collapsing the two flags into a single parameter,
as we never have a caller setting both flags at once.
Ultimately, we want to also unbox branches for QAPI unions, but as
that touches a lot more client code, it is better as separate
patches. But since unions and alternates share gen_variants(), I
had to hack in a way to test if we are visiting an alternate type
for setting the is_unboxed flag: look for a non-object branch.
This works because alternates have at least two branches, with at
most one object branch, while unions have only object branches.
The hack will go away in a later patch.
The generated code difference to qapi-types.h is relatively small:
| struct BlockdevRef {
| QType type;
| union { /* union tag is @type */
| void *data;
|- BlockdevOptions *definition;
|+ BlockdevOptions definition;
| char *reference;
| } u;
| };
The corresponding spot in qapi-visit.c calls visit_type_FOO(), which
first calls visit_start_struct() to allocate or deallocate the member
and handle a layer of {} from the JSON stream, then visits the
members. To peel off the indirection and the memory management that
comes with it, we inline this call, then suppress allocation /
deallocation by passing NULL to visit_start_struct(), and adjust the
member visit:
| switch ((*obj)->type) {
| case QTYPE_QDICT:
|- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
|+ visit_start_struct(v, name, NULL, 0, &err);
|+ if (err) {
|+ break;
|+ }
|+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err);
|+ error_propagate(errp, err);
|+ err = NULL;
|+ visit_end_struct(v, &err);
| break;
| case QTYPE_QSTRING:
| visit_type_str(v, name, &(*obj)->u.reference, &err);
The visit of non-object fields is unchanged.
Backports commit becceedc4d9bc1435099c90a0514945a89844d3a from qemu
We have several instances of methods that do an early exit if
output is not needed, then log that output is being generated,
and finally produce the output; see qapi-types.py:gen_object()
and qapi-visit.py:gen_visit_implicit_struct(). The odd man
out was gen_visit_fields_decl(); rearrange it to be more like
the others. No semantic change or difference to generated code.
Backports commit 2208d64998c5f867ccee7eeee298971685bf822d from qemu
We were passing 'Foo **obj' to the internal helper function, but
all uses within the helper were via reads of '*obj'. Refactor
things to pass one less level of indirection, by having the
callers dereference before calling.
For an example of the generated code change:
|-static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo **obj, Error **errp)
|+static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo *obj, Error **errp)
| {
| Error *err = NULL;
|
|- visit_type_int(v, "actual", &(*obj)->actual, &err);
|+ visit_type_int(v, "actual", &obj->actual, &err);
| error_propagate(errp, err);
| }
|
|@@ -261,7 +261,7 @@ void visit_type_BalloonInfo(Visitor *v,
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_BalloonInfo_fields(v, obj, &err);
|+ visit_type_BalloonInfo_fields(v, *obj, &err);
| out_obj:
The refactoring will also make it easier to reuse the helpers in
a future patch when implicit structs are stored directly in the
parent struct rather than boxed through a pointer.
Backports commit 655519030b5d20967ae3afa1fe91ef5ad4406065 from qemu
gen_visit_union() is now just like gen_visit_struct(). Rename
it to gen_visit_object(), use it for structs, and drop
gen_visit_struct(). Output is unchanged.
Backports commit 59d9e84cc94b8268f13ec184acfb997e8f352593 from qemu
We initially created the static visit_type_FOO_fields() helper
function for reuse of code - we have cases where the initial
setup for a visit has different allocation (depending on whether
the fields represent a stand-alone type or are embedded as part
of a larger type), but where the actual field visits are
identical once a pointer is available.
Up until the previous patch, visit_type_FOO_fields() was only
used for structs (no variants), so it was covering every field
for each type where it was emitted.
Meanwhile, the code for visiting unions looks like:
static visit_type_U_fields() {
visit base;
visit local_members;
}
visit_type_U() {
visit_start_struct();
visit_type_U_fields();
visit variants;
visit_end_struct();
}
which splits the fields of the union visit across two functions.
Move the code to visit variants to live inside visit_type_U_fields(),
while making it conditional on having variants so that all other
instances of the helper function remain unchanged. This is also
a step closer towards unifying struct and union visits, and towards
allowing one union type to be the branch of another flat union.
The resulting diff to the generated code is a bit hard to read,
but it can be verified that it touches only union types, and that
the end result is the following general structure:
static visit_type_U_fields() {
visit base;
visit local_members;
visit variants;
}
visit_type_U() {
visit_start_struct();
visit_type_U_fields();
visit_end_struct();
}
Backports commit 9a5cd424d5f06fb5293eb264456d89343c557558 from qemu
For a simple union SU, gen_visit_union() generates a visit of its
single tag member, like this:
visit_type_SUKind(v, "type", &(*obj)->type, &err);
For a flat union FU with base B, it generates a visit of its base
fields:
visit_type_B_fields(v, (B **)obj, &err);
Instead, we can simply visit the common members using the same fields
visit function we use for structs, generated with
gen_visit_struct_fields(). This function visits the base if any, then
the local members.
For a simple union SU, visit_type_SU_fields() contains exactly the old
tag member visit, because there is no base, and the tag member is the
only member. For instance, the code generated for qapi-schema.json's
KeyValue changes like this:
+static void visit_type_KeyValue_fields(Visitor *v, KeyValue **obj, Error **errp)
+{
+ Error *err = NULL;
+
+ visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
+ if (err) {
+ goto out;
+ }
+
+out:
+ error_propagate(errp, err);
+}
+
void visit_type_KeyValue(Visitor *v, const char *name, KeyValue **obj, Error **errp)
{
Error *err = NULL;
@@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor *v, con
if (!*obj) {
goto out_obj;
}
- visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
+ visit_type_KeyValue_fields(v, obj, &err);
if (err) {
goto out_obj;
}
For a flat union FU, visit_type_FU_fields() contains exactly the old
base fields visit, because there is a base, but no members. For
instance, the code generated for qapi-schema.json's CpuInfo changes
like this:
static void visit_type_CpuInfoBase_fields(Visitor *v, CpuInfoBase **obj, Error **errp);
+static void visit_type_CpuInfo_fields(Visitor *v, CpuInfo **obj, Error **errp)
+{
+ Error *err = NULL;
+
+ visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
+ if (err) {
+ goto out;
+ }
+
+out:
+ error_propagate(errp, err);
+}
+
static void visit_type_CpuInfoX86_fields(Visitor *v, CpuInfoX86 **obj, Error **errp)
...
@@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor *v, cons
if (!*obj) {
goto out_obj;
}
- visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
+ visit_type_CpuInfo_fields(v, obj, &err);
if (err) {
goto out_obj;
}
As you see, the generated code grows a bit, but in practice, it's lost
in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%.
This simplification became possible with commit 441cbac "qapi-visit:
Convert to QAPISchemaVisitor, fixing bugs". It's a step towards
unifying gen_struct() and gen_union().
Backports commit d7445b57f4342d50c25b5bc1746048fa3720511b from qemu
When we added support for a user-specified prefix for an enum
type (commit 351d36e), we forgot to teach the qapi-visit code
to honor that prefix in the case of using a prefixed enum as
the discriminator for a flat union. While there is still some
on-list debate on whether we want to keep prefixes, we should
at least make it work as long as it is still part of the code
base.
Backports commit 9d3524b39e1fe5f3bb7a990ad7841e469e954a3b from qemu
visit_start_struct() and visit_type_enum() had a 'kind' argument
that was usually set to either the stringized version of the
corresponding qapi type name, or to NULL (although some clients
didn't even get that right). But nothing ever used the argument.
It's even hard to argue that it would be useful in a debugger,
as a stack backtrace also tells which type is being visited.
Therefore, drop the 'kind' argument as dead.
Backports commit 337283dffbb5ad5860ed00408a5fd0665c21be07 from qemu
JSON uses "name":value, but many of our visitor interfaces were
called with visit_type_FOO(v, &value, name, errp). This can be
a bit confusing to have to mentally swap the parameter order to
match JSON order. It's particularly bad for visit_start_struct(),
where the 'name' parameter is smack in the middle of the
otherwise-related group of 'obj, kind, size' parameters! It's
time to do a global swap of the parameter ordering, so that the
'name' parameter is always immediately after the Visitor argument.
Additional reason in favor of the swap: the existing include/qjson.h
prefers listing 'name' first in json_prop_*(), and I have plans to
unify that file with the qapi visitors; listing 'name' first in
qapi will minimize churn to the (admittedly few) qjson.h clients.
Later patches will then fix docs, object.h, visitor-impl.h, and
those clients to match.
Done by first patching scripts/qapi*.py by hand to make generated
files do what I want, then by running the following Coccinelle
script to affect the rest of the code base:
$ spatch --sp-file script `git grep -l '\bvisit_' -- '**/*.[ch]'`
I then had to apply some touchups (Coccinelle insisted on TAB
indentation in visitor.h, and botched the signature of
visit_type_enum() by rewriting 'const char *const strings[]' to
the syntactically invalid 'const char*const[] strings'). The
movement of parameters is sufficient to provoke compiler errors
if any callers were missed.
// Part 1: Swap declaration order
@@
type TV, TErr, TObj, T1, T2;
identifier OBJ, ARG1, ARG2;
@@
void visit_start_struct
-(TV v, TObj OBJ, T1 ARG1, const char *name, T2 ARG2, TErr errp)
+(TV v, const char *name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp)
{ ... }
@@
type bool, TV, T1;
identifier ARG1;
@@
bool visit_optional
-(TV v, T1 ARG1, const char *name)
+(TV v, const char *name, T1 ARG1)
{ ... }
@@
type TV, TErr, TObj, T1;
identifier OBJ, ARG1;
@@
void visit_get_next_type
-(TV v, TObj OBJ, T1 ARG1, const char *name, TErr errp)
+(TV v, const char *name, TObj OBJ, T1 ARG1, TErr errp)
{ ... }
@@
type TV, TErr, TObj, T1, T2;
identifier OBJ, ARG1, ARG2;
@@
void visit_type_enum
-(TV v, TObj OBJ, T1 ARG1, T2 ARG2, const char *name, TErr errp)
+(TV v, const char *name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp)
{ ... }
@@
type TV, TErr, TObj;
identifier OBJ;
identifier VISIT_TYPE =~ "^visit_type_";
@@
void VISIT_TYPE
-(TV v, TObj OBJ, const char *name, TErr errp)
+(TV v, const char *name, TObj OBJ, TErr errp)
{ ... }
// Part 2: swap caller order
@@
expression V, NAME, OBJ, ARG1, ARG2, ERR;
identifier VISIT_TYPE =~ "^visit_type_";
@@
(
-visit_start_struct(V, OBJ, ARG1, NAME, ARG2, ERR)
+visit_start_struct(V, NAME, OBJ, ARG1, ARG2, ERR)
|
-visit_optional(V, ARG1, NAME)
+visit_optional(V, NAME, ARG1)
|
-visit_get_next_type(V, OBJ, ARG1, NAME, ERR)
+visit_get_next_type(V, NAME, OBJ, ARG1, ERR)
|
-visit_type_enum(V, OBJ, ARG1, ARG2, NAME, ERR)
+visit_type_enum(V, NAME, OBJ, ARG1, ARG2, ERR)
|
-VISIT_TYPE(V, OBJ, NAME, ERR)
+VISIT_TYPE(V, NAME, OBJ, ERR)
)
Backports commit 51e72bc1dd6ace6e91d675f41a1f09bd00ab8043 from qemu
C compilers are allowed to represent enums as a smaller type
than int, if all enum values fit in the smaller type. There
are even compiler flags that force the use of this smaller
representation, although using them changes the ABI of a
binary. Therefore, our generated code for visit_type_ENUM()
(for all qapi enums) was wrong for casting Enum* to int* when
calling visit_type_enum().
It appears that no one has been using compiler ABI switches
for qemu, because if they had, we are potentially dereferencing
beyond bounds or even risking a SIGBUS on platforms where
unaligned pointer dereferencing is fatal. But it is still
better to avoid the practice entirely, and just use the correct
types.
This matches the fix for alternate qapi types, done earlier in
commit 0426d53 "qapi: Simplify visiting of alternate types",
with generated code changing as:
| void visit_type_QType(Visitor *v, QType *obj, const char *name, Error **errp)
| {
|- visit_type_enum(v, (int *)obj, QType_lookup, "QType", name, errp);
|+ int value = *obj;
|+ visit_type_enum(v, &value, QType_lookup, "QType", name, errp);
|+ *obj = value;
| }
Backports commit 395a233f7c089f23e3c0d43ce34c709dc5acd7de from qemu
The generated code can call visit_end_union() without having called
visit_start_union(). Example:
if (!*obj) {
goto out_obj;
}
visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
if (err) {
goto out_obj; // if we go from here...
}
if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
goto out_obj;
}
switch ((*obj)->arch) {
[...]
}
out_obj:
// ... then *obj is true, and ...
error_propagate(errp, err);
err = NULL;
if (*obj) {
// we end up here
visit_end_union(v, !!(*obj)->u.data, &err);
}
error_propagate(errp, err);
Harmless only because no visitor implements end_union(). Clean it up
anyway, by deleting the function as useless.
Messed up since we have visit_end_union (commit cee2ded).
Backports commit 7c91aabd8964cfdf637f302c579c95401f21ce92 from qemu
Inside the generated code between visit_start_struct() and
visit_end_struct(), we were blindly setting the error into
the caller's errp parameter. But a future patch to split
visit_end_struct() will require that we take action based
on whether an error has occurred, which requires us to track
all actions through a local err. Rewrite the visits to be
more in line with the other generated calls.
Generated code changes look like:
| visit_start_struct(v, (void **)obj, "Abort", name, sizeof(Abort), &err);
|- if (!err) {
|- if (*obj) {
|- visit_type_Abort_fields(v, obj, errp);
|- }
|- visit_end_struct(v, &err);
|+ if (err) {
|+ goto out;
| }
|+ if (!*obj) {
|+ goto out_obj;
|+ }
|+ visit_type_Abort_fields(v, obj, &err);
|+ error_propagate(errp, err);
|+ err = NULL;
|+out_obj:
|+ visit_end_struct(v, &err);
|+out:
| error_propagate(errp, err);
| }
Backports commit 92b09babc11b60458c28cfe37eaa314de50e6241 from qemu
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.
Backports commit d00341af384665d259af475b14c96bb8414df415 from qemu
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).
Backports commit 0426d53c6530606bf7641b83f2b755fe61c280ee from qemu
What's more meta than using qapi to define qapi? :)
Convert QType into a full-fledged[*] builtin qapi enum type, so
that a subsequent patch can then use it as the discriminator
type of qapi alternate types. Fortunately, the judicious use of
'prefix' in the qapi definition avoids churn to the spelling of
the enum constants.
To avoid circular definitions, we have to flip the order of
inclusion between "qobject.h" vs. "qapi-types.h". Back in commit
28770e0, we had the latter include the former, so that we could
use 'QObject *' for our implementation of 'any'. But that usage
also works with only a forward declaration, whereas the
definition of QObject requires QType to be a complete type.
[*] The type has to be builtin, rather than declared in
qapi/common.json, because we want to use it for alternates even
when common.json is not included. But since it is the first
builtin enum type, we have to add special cases to qapi-types
and qapi-visit to only emit definitions once, even when two
qapi files are being compiled into the same binary (the way we
already handled builtin list types like 'intList'). We may
need to revisit how multiple qapi files share common types,
but that's a project for another day.
Backports commit 7264f5c50cc1be0f1406e3ebb45aedcca02f603a from qemu
We were previously creating all unions with an empty list for
local_members. However, it will make it easier to unify struct
and union generation if we include the generated tag member in
local_members. That way, we can have a common code pattern:
visit the base (if any), visit the local members (if any), visit
the variants (if any). The local_members of a flat union
remains empty (because the discriminator is already visited as
part of the base). Then, by visiting tag_member.check() during
AlternateType.check(), we no longer need to call it during
Variants.check().
The various front end entities now exist as follows:
struct: optional base, optional local_members, no variants
simple union: no base, one-element local_members, variants with tag_member
from local_members
flat union: base, no local_members, variants with tag_member from base
alternate: no base, no local_members, variants
With the new local members, we require a bit of finesse to
avoid assertions in the clients.
No change to generated code.
Backports commit da34a9bd999d1be13954c699bbae0295cdaa3200 from qemu
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.
Backports commit dd5ee2c2d3e3a17647ddd9bfa97935b8cb5dfa40 from qemu
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 qapi-visit.py.
Generated code changes look like:
|@@ -4912,16 +4912,16 @@ void visit_type_MemoryDeviceInfo(Visitor
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_MemoryDeviceInfoKind(v, &(*obj)->kind, "type", &err);
|+ visit_type_MemoryDeviceInfoKind(v, &(*obj)->type, "type", &err);
| if (err) {
| goto out_obj;
| }
|- if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
|+ if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
| goto out_obj;
| }
|- switch ((*obj)->kind) {
|+ switch ((*obj)->type) {
| case MEMORY_DEVICE_INFO_KIND_DIMM:
|- visit_type_PCDIMMDeviceInfo(v, &(*obj)->dimm, "data", &err);
|+ visit_type_PCDIMMDeviceInfo(v, &(*obj)->u.dimm, "data", &err);
| break;
| default:
| abort();
|@@ -4930,7 +4930,7 @@ out_obj:
| error_propagate(errp, err);
| err = NULL;
| if (*obj) {
|- visit_end_union(v, !!(*obj)->data, &err);
|+ visit_end_union(v, !!(*obj)->u.data, &err);
| }
| error_propagate(errp, err);
| err = NULL;
Backports commit 150d0564a4c626642897c748f7906260a13c14e1 from qemu
The code for visiting the base class of a child struct created
visit_type_Base_fields() which covers all fields of Base; while
the code for visiting the base class of a flat union created
visit_type_Union_fields() covering all fields of the base
except the discriminator. But since the base class includes
the discriminator of a flat union, we can just visit the entire
base, without needing a separate visit of the discriminator.
Not only is consistently visiting all fields easier to
understand, it lets us share code.
The generated code in qapi-visit.c loses several now-unused
visit_type_UNION_fields(), along with changes like:
|@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_BlockdevOptions_fields(v, obj, &err);
|- if (err) {
|- goto out_obj;
|- }
|- visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err);
|+ visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj, &err);
| if (err) {
| goto out_obj;
| }
and forward declarations where needed. Note that the cast of obj
to BASE ** is necessary to call visit_type_BASE_fields() (and we
can't use our upcast wrappers, because those work on pointers while
we have a pointer-to-pointer).
Backports commit 5c5e51a05b567fd48fb155d94ca6f7679dd0d478 from qemu
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.
Backports commit ddf21908961073199f3d186204da4810f2ea150b from qemu
We generate a static visit_type_FOO_fields() for every type
FOO. However, sometimes we need a forward declaration. Split
the code to generate the forward declaration out of
gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
and also prepare for a forward declaration to be emitted
during gen_visit_struct(), so that a future patch can switch
from using visit_type_FOO_implicit() to the simpler
visit_type_FOO_fields() as part of unboxing the base class
of a struct.
No change to generated code.
Backports commit d02cf37766ba3cf918d7085aa7848c9dc05fd11a from qemu
We were using regular expressions to see if ret included
any earlier text that emitted a 'goto out;' line, to decide
whether we needed to output an 'out:' label. But this is
fragile, if the ret text can possibly combine more than one
generated function body, where the first function used a
goto but the second does not. Change the code to just check
for the known conditions which cause an error check to be
needed. Besides, it's slightly more efficient to use plain
checks than regular expression searching.
No change to generated code.
Backports commit f9e6102b48f21e464a847a858a456c521e7a83e5 from qemu
A future patch will enable error reporting from the various
QAPISchema*.check() methods. But to report an error related
to an implicit type, we'll need to associate a location with
the type (the same location as the top-level entity that is
causing the creation of the implicit type), and once we do
that, keying off of whether foo.info exists is no longer a
viable way to determine if foo is an implicit type.
Instead, add an is_implicit() method to QAPISchemaEntity, and use it.
It can be overridden later for ObjectType and EnumType, when implicit
instances of those classes gain info.
Backports commit 49823c4b4304a3e4aa5d67e089946b12d6a52d64 from qemu
Previously, qapi-types and qapi-visit filtered out implicit
objects during visit_object_type() by using 'info' (works since
implicit objects do not [yet] have associated info); meanwhile
qapi-introspect filtered out all schema types on the first pass
by returning a python type from visit_begin(), which was then
used at a distance in QAPISchema.visit() to do the filtering.
Rather than keeping these ad hoc approaches, add a new visitor
callback visit_needed() which returns False to skip a given
entity, and which defaults to True unless overridden. Use the
new mechanism to simplify all three filtering visitors.
No change to the generated code.
Backports commit 25a0d9c977c2f5db914b0a1619759fd77d97b016 from qemu
Consolidate the code between visit, command marshalling, and
event generation that iterates over the members of a struct.
It reduces code duplication in the generator, so that a future
patch can reduce the size of generated code while touching only
one instead of three locations.
There are no changes to the generated marshal code.
The visitor code becomes slightly more verbose, but remains
semantically equivalent, and is actually easier to read as
it follows a more common idiom:
| visit_optional(v, &(*obj)->has_device, "device", &err);
|- if (!err && (*obj)->has_device) {
|- visit_type_str(v, &(*obj)->device, "device", &err);
|- }
| if (err) {
| goto out;
| }
|+ if ((*obj)->has_device) {
|+ visit_type_str(v, &(*obj)->device, "device", &err);
|+ if (err) {
|+ goto out;
|+ }
|+ }
The event code becomes slightly more verbose, but this is
arguably a bug fix: although the visitors are not well
documented, use of an optional member should not be attempted
unless guarded by a prior call to visit_optional(). Works only
because the output qmp visitor has a no-op visit_optional():
|+ visit_optional(v, &has_offset, "offset", &err);
|+ if (err) {
|+ goto out;
|+ }
| if (has_offset) {
| visit_type_int(v, &offset, "offset", &err);
Backports commit 82ca8e469666b169ccf818a0e36136aee97d7db0 from qemu
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is one patch of a series to clean up these differences.
This patch reduces the number of push_indent()/pop_indent() pairs
so that generated code is typically already at its natural output
indentation in the python files. It is easier to reason about
generated code if the reader does not have to track how much
spacing will be inserted alongside the code, and moreso when all
of the generators use the same patterns (qapi-type and qapi-event
were already using in-place indentation).
Arguably, the resulting python may be a bit harder to read with C
code at the same indentation as python; on the other hand, not
having to think about push_indent() is a win, and most decent
editors provide syntax highlighting that makes it easier to
visually distinguish python code from string literals that will
become C code.
There is no change to the generated output.
Backports commit 05372f708a8cb3556e4d67458de79417dadf241f from qemu
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is one patch of a series to clean up these differences.
This patch adjusts gen_visit_union() to use the same indentation
as other functions, namely, by jumping early to the error label
if the object was not set rather than placing the rest of the
body inside an if for when it is set.
No change in semantics to the generated code.
Backports commit e36c714e6aad7c9266132350833e2f263f6d8874 from qemu
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is one patch of a series to clean up these differences.
This patch names the goto labels 'out' (not 'clean') and 'out_obj'
(not 'out_end'). Additionally, the generator was inconsistent on
whether labels had a leading space [our HACKING is silent; while
emacs 'gnu' style adds the space to avoid littering column 1].
For minimal churn, prefer no leading space; this also matches
the style that is more prevalent in current qemu.git.
No change in semantics to the generated code.
Backports commit f782399cb4fa3fc4182cb046817f65a6db92ab07 from qemu
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is one patch of a series to clean up these differences.
This patch names the local visitor variable 'v' rather than 'm'.
Related objects, such as 'QapiDeallocVisitor', are also named by
their initials instead of an unrelated leading m.
No change in semantics to the generated code.
Backports commit f8b7f1a8eafa9f565ebecfe409e8741d38cd786b from qemu
Generate just 'FOO' instead of 'struct FOO' when possible.
Drop helper functions that are now unused.
Make pep8 and pylint reasonably happy.
Rename generate_FOO() functions to gen_FOO() for consistency.
Use more consistent and sensible variable names.
Consistently use c_ for mapping keys when their value is a C
identifier or type.
Simplify gen_enum() and gen_visit_union()
Consistently use single quotes for C text string literals.
Backports commit e98859a9b96d71dea8f9af43325edd43c7effe66 from qemu
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.
Backports commit 441cbac0c7e641780decbc674a9a68c6a5200f71 from qemu
The QAPI code generators work with a syntax tree (nested dictionaries)
plus a few symbol tables (also dictionaries) on the side.
They have clearly outgrown these simple data structures. There's lots
of rummaging around in dictionaries, and information is recomputed on
the fly. For the work I'm going to do, I want more clearly defined
and more convenient interfaces.
Going forward, I also want less coupling between the back-ends and the
syntax tree, to make messing with the syntax easier.
Create a bunch of classes to represent QAPI schemata.
Have the QAPISchema initializer call the parser, then walk the syntax
tree to create the new internal representation, and finally perform
semantic analysis.
Shortcut: the semantic analysis still relies on existing check_exprs()
to do the actual semantic checking. All this code needs to move into
the classes. Mark as TODO.
Simple unions are lowered to flat unions. Flat unions and structs are
represented as a more general object type.
Catching name collisions in generated code would be nice. Mark as
TODO.
We generate array types eagerly, even though most of them aren't used.
Mark as TODO.
Nothing uses the new intermediate representation just yet, thus no
change to generated files.
Backports commit ac88219a6c78302c693fb60fe6cf04358540fbce from qemu
Clean up white-space, brace placement, and superfluous #ifdef
QAPI_TYPES_BUILTIN_CLEANUP_DEF.
Backports commit 3a864e7c52af15017d5082a9ee39a7919f46d2b5 from qemu
The generated code passes mangled schema names to visit_type_enum()
and union's visit_start_struct(). Fix it to pass the names
unadulterated, like we do everywhere else.
Only qapi-schema-test.json actually has names where this makes a
difference: enum __org.qemu_x-Enum, flat union __org.qemu_x-Union2,
simple union __org.qemu_x-Union1 and its implicit enum
__org.qemu_x-Union1Kind.
Backports commit 40b3adec13a9e022ff5a2e2b81c243fc0a026746 from qemu
The visit_type_implicit_FOO() are generated on demand, right before
their first use. Used by visit_type_STRUCT_fields() when STRUCT has
base FOO, and by visit_type_UNION() when flat UNION has member a FOO.
If the schema defines FOO after its first use as struct base or flat
union member, visit_type_implicit_FOO() calls
visit_type_implicit_FOO() before its definition, which doesn't
compile.
Rearrange qapi-schema-test.json to demonstrate the bug.
Fix by generating the necessary forward declaration.
Backports commit 8c3f8e77215bfedb7854221868f655e148506936 from qemu
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.
Backports commit 0f61af3eb396ae163cd1572ce12e05f5d08d7c15 from qemu
gen_sync_call()'s parameter indent is useless: gen_sync_call() uses it
only as optional argument for push_indent() and pop_indent(), their
default is four, and gen_sync_call()'s only caller passes four. Drop
the parameter.
gen_visitor_input_containers_decl()'s parameter obj is always
"QOBJECT(args)". Use that, and drop the parameter.
Drop unused parameters of gen_marshal_output(),
gen_marshal_input_decl(), generate_visit_struct_body(),
generate_visit_list(), generate_visit_enum(), generate_declaration(),
generate_enum_declaration(), generate_decl_enum().
Drop unused variables in generate_event_enum_lookup(),
generate_enum_lookup(), generate_visit_struct_fields(), check_event().
Backports commit 5aa05d3f72e556752167f7005d6a3dea0f4432c5 from qemu
Enhance the testsuite to cover downstream alternates, including
whether the branch name or type is downstream. Update the
generator to mangle alternate names in the appropriate places.
Backports commit d1f07c86c05706facf950b0b0dba370f71fd5ef6 from qemu
Enhance the testsuite to cover downstream flat unions, including
the base type, discriminator name and type, and branch name and
type. Update the generator to mangle the union names in the
appropriate places.
Backports commit 857af5f06c3fb097d1bb6bc8a23b9992aac99e75 from qemu
Enhance the testsuite to cover downstream simple unions, including
when a union branch is a downstream name. Update the generator to
mangle the union names in the appropriate places.
Backports commit bb33729043ceda56b4068db13bdc17786ebd0ed0 from qemu
Enhance the testsuite to cover downstream structs, including struct
members and base structs. Update the generator to mangle the
struct names in the appropriate places.
Backports commit 83a02706bb1fd31c93eab755de543dfe228682d4 from qemu
Enhance the testsuite to cover a downstream enum type and enum
string. Update the generator to mangle the enum name in the
appropriate places.
Backports commit fce384b8e5193e02421f6b2c2880f3684abcbdc0 from qemu
Now that the two functions are identical, we only need one of them,
and we might as well give it a more descriptive name. Basically,
the function serves as the translation from a QAPI name into a
(portion of a) C identifier, without regards to whether it is a
variable or function name.
Backports commit 18df515ebbefa9f13474b128b8050d5fa346ea1e from qemu
Now that we no longer have nested structs to visit, the use of
prefix strings is no longer required. Remove the code that is
no longer reachable.
Backports commit a82b982e2bddf7cd7cb490f83643e952e17d4523 from qemu
A future patch will be using a 'name':{dictionary} entry in the
QAPI schema to specify a default value for an optional argument
(see previous commit messages for more details why); but existing
use of inline nested structs conflicts with that goal. Now that
all commands have been changed to avoid inline nested structs,
nuke support for them, and turn it into a hard error. Update the
testsuite to reflect tighter parsing rules.
Backports commit 6b5abc7df7ef9aadb3ff0eba6ccf4f1f0181e2e1 from qemu
