There's no reason to do two malloc's for a flat union; let's just
inline the branch struct directly into the C union branch of the
flat union.
Surprisingly, fewer clients were actually using explicit references
to the branch types in comparison to the number of flat unions
thus modified.
This lets us reduce the hack in qapi-types:gen_variants() added in
the previous patch; we no longer need to distinguish between
alternates and flat unions.
The change to unboxed structs means that u.data (added in commit
cee2dedb) is now coincident with random fields of each branch of
the flat union, whereas beforehand it was only coincident with
pointers (since all branches of a flat union have to be objects).
Note that this was already the case for simple unions - but there
we got lucky. Remember, visit_start_union() blindly returns true
for all visitors except for the dealloc visitor, where it returns
the value !!obj->u.data, and that this result then controls
whether to proceed with the visit to the variant. Pre-patch,
this meant that flat unions were testing whether the boxed pointer
was still NULL, and thereby skipping visit_end_implicit_struct()
and avoiding a NULL dereference if the pointer had not been
allocated. The same was true for simple unions where the current
branch had pointer type, except there we bypassed visit_type_FOO().
But for simple unions where the current branch had scalar type, the
contents of that scalar meant that the decision to call
visit_type_FOO() was data-dependent - the reason we got lucky there
is that visit_type_FOO() for all scalar types in the dealloc visitor
is a no-op (only the pointer variants had anything to free), so it
did not matter whether the dealloc visit was skipped. But with this
patch, we would risk leaking memory if we could skip a call to
visit_type_FOO_fields() based solely on a data-dependent decision.
But notice: in the dealloc visitor, visit_type_FOO() already handles
a NULL obj - it was only the visit_type_implicit_FOO() that was
failing to check for NULL. And now that we have refactored things to
have the branch be part of the parent struct, we no longer have a
separate pointer that can be NULL in the first place. So we can just
delete the call to visit_start_union() altogether, and blindly visit
the branch type; there is no change in behavior except to the dealloc
visitor, where we now unconditionally visit the branch, but where that
visit is now always safe (for a flat union, we can no longer
dereference NULL, and for a simple union, visit_type_FOO() was already
safely handling NULL on pointer types).
Unfortunately, simple unions are not as easy to switch to unboxed
layout; because we are special-casing the hidden implicit type with
a single 'data' member, we really DO need to keep calling another
layer of visit_start_struct(), with a second malloc; although there
are some cleanups planned for simple unions in later patches.
visit_start_union() and gen_visit_implicit_struct() are now unused.
Drop them.
Note that after this patch, the only remaining use of
visit_start_implicit_struct() is for alternate types; the next patch
will do further cleanup based on that fact.
Backports commit 544a3731591f5d53e15f22de00ce5ac758d490b3 from qemu
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
Right now, we emit the branches of union types as a boxed pointer,
and it suffices to have a forward declaration of the type. However,
a future patch will swap things to directly use the branch type,
instead of hiding it behind a pointer. For this to work, the
compiler needs the full definition of the type, not just a forward
declaration, prior to the union that is including the branch type.
This patch just adds topological sorting to hoist all types
mentioned in a branch of a union to be fully declared before the
union itself. The sort is always possible, because we do not
allow circular union types that include themselves as a direct
branch (it is, however, still possible to include a branch type
that itself has a pointer to the union, for a type that can
indirectly recursively nest itself - that remains safe, because
that the member of the branch type will remain a pointer, and the
QMP representation of such a type adds another {} for each recurring
layer of the union type).
Backports commit 1de5d4ca0752138034305f3d4e8fe17ef6503569 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
The whole point of an alternate is to allow some type-safety while
still accepting more than one JSON type. Meanwhile, the 'any'
type exists to bypass type-safety altogether. The two are
incompatible: you can't accept every type, and still tell which
branch of the alternate to use for the parse; fix this to give a
sane error instead of a Python stack trace.
Note that other types that can't be alternate members are caught
earlier, by check_type().
Backports commit 46534309e667fd860720f983c2c9aefe0354340d from qemu
Empty unions serve no purpose, and while we compile with gcc
which permits them, strict C99 forbids them. We happen to inject
a dummy 'void *data' member into the C unions that represent QAPI
unions and alternates, but we want to get rid of that member (it
pollutes the namespace for no good reason), which would leave us
with an empty union if the user didn't provide any branches. While
empty structs make sense in QAPI, empty unions don't add any
expressiveness to the QMP language. So prohibit them at parse
time. Update the documentation and testsuite to match.
Note that the documentation already mentioned that alternates
should have "two or more JSON data types"; so this also fixes
the code to enforce that. However, we have existing uses of a
union type with only one branch, so the 2-or-more strictness
is intentionally limited to alternates.
Backports commit 02a57ae32b08e8981b59979b80e682c9a153e94d 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
Commit 86f4b687 broke compilation on MIPS and SPARC, which have a
preprocessor pollution of '#define mips 1' and '#define sparc 1',
respectively. Treat it the same way as we do for the pollution with
'unix', so that QMP remains backwards compatible and only the C code
needs to use the alternative 'q_mips', 'q_sparc' spelling.
Backports commit 86ae191163d48ff4ff9d9996868e6cfe92a82449 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
All other successful clients of visit_start_struct() were paired
with an unconditional visit_end_struct(); but the generated
code for events was relying on qmp_output_visitor_cleanup() to
work on an incomplete visit. Alter the code to guarantee that
the struct is completed, which will make a future patch to
split visit_end_struct() easier to reason about. While at it,
drop some assertions and comments that are not present in other
uses of the qmp output visitor, and pass NULL rather than "" as
the 'kind' parameter (matching most other uses where obj is NULL).
The changes to the generated code look like:
| qmp = qmp_event_build_dict("DEVICE_TRAY_MOVED");
|
| qov = qmp_output_visitor_new();
|- g_assert(qov);
|-
| v = qmp_output_get_visitor(qov);
|- g_assert(v);
|
|- /* Fake visit, as if all members are under a structure */
|- visit_start_struct(v, NULL, "", "DEVICE_TRAY_MOVED", 0, &err);
|+ visit_start_struct(v, NULL, NULL, "DEVICE_TRAY_MOVED", 0, &err);
| if (err) {
| goto out;
| }
| visit_type_str(v, (char **)&device, "device", &err);
| if (err) {
|- goto out;
|+ goto out_obj;
| }
| visit_type_bool(v, &tray_open, "tray-open", &err);
| if (err) {
|- goto out;
|+ goto out_obj;
| }
|- visit_end_struct(v, &err);
|+out_obj:
|+ visit_end_struct(v, err ? NULL : &err);
| if (err) {
| goto out;
| }
|
| obj = qmp_output_get_qobject(qov);
|- g_assert(obj != NULL);
|+ g_assert(obj);
|
| qdict_put_obj(qmp, "data", obj);
| emit(QAPI_EVENT_DEVICE_TRAY_MOVED, qmp, &err);
Note that the 'goto out_obj' with no intervening code before the
label, as well as the construct of 'err ? NULL : &err', are both
a bit unusual but also temporary; they get fixed in a later patch
that splits visit_end_struct() to drop its errp parameter by moving
some checking before the label. But until that time, this was the
simplest way to avoid the appearance of passing a possibly-set
error to visit_end_struct(), even though actual code inspection
shows that visit_end_struct() for a QMP output visitor will never
set an error.
Backports commit a16e3e5c5825c90887a863513916f93eeec16c55 from qemu
Commit 5cdc8831 reworked gen_params() to be simpler, but forgot
to clean up a now-unused errp named argument.
No change to generated code.
Backports commit e408311546b633b232312450d9017f4db21df0dc from qemu
PEP 8 calls for it, because it's forward compatible with Python 3.
Supported since Python 2.6, which we require (commit fec2103).
Backports commit 291928a80f39670929fcce222acd5e21d1434692 from qemu
It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child. But the
old check_member_clash() parser function was not prepared to
check for this, and entered an infinite recursion (at least
until Python gives up, complaining about nesting too deep).
Now that check_member_clash() has been recently removed,
attempts at self-inheritance trigger an assertion failure
introduced by commit ac88219a. The obvious fix is to turn
the assertion into a conditional.
This patch includes both the tests (base-cycle-direct and
base-cycle-indirect) and the fix, since the .err file output
for the unfixed case is not useful (particularly when it was
warning about unbounded recursion, as that limit may be
platform-specific).
We don't need to worry about cycles in flat unions (neither
the base type nor the type of a variant can be a union) nor
in alternates (alternate branches cannot themselves be an
alternate). But if we later allow a union type as a variant,
we will still be okay, as QAPISchemaObjectTypeVariants.check()
triggers the same QAPISchemaObjectType.check() that will
detect any loops.
Likewise, we need not worry about the case of diamond
inheritance where the same class is used for a flat union base
class and one of its variants; either both uses will introduce
a collision in trying to insert the same member name twice, or
the shared type is empty and changes nothing.
Backports commit bac5429ccb4f41d421ec641b11f1852c8420fdb7 from qemu
With the recent commit 'qapi: Detect collisions in C member
names', we have two different locations for detecting clashes -
one at parse time, and another at QAPISchema*.check() time.
Remove all of the ad hoc parser checks, and delete associated
code (for example, the global check_member_clash() method is
no longer needed).
Testing this showed that the test union-bad-branch wasn't adding
much: union-clash-branches also exposes the error message when
branches collide, and we've recently fixed things to avoid an
implicit collision with max. Likewise, the error for
enum-clash-member changes to report our new detection of
upper case in a value name, unless we modify the test to use
all lower case.
The wording of several error messages has changed, but the
change is generally an improvement rather than a regression.
No change to generated code.
Backports commit 01cfbaa4c36ecd9f1c7bcbad50c92758e1d147c4 from qemu
We document that members of enums and objects should be
'lower-case', although we were not enforcing it. We have to
whitelist a few pre-existing entities that violate the norms.
Add three new tests to expose the new error message, each of
which first uses the whitelisted name 'UuidInfo' to prove the
whitelist works, then triggers the failure (this is the same
pattern used in the existing returns-whitelist.json test).
Note that by adding this check, we have effectively forbidden
an entity with a case-insensitive clash of member names, for
any entity that is not on the whitelist (although there is
still the possibility to clash via '-' vs. '_').
Not done here: a future patch should also add naming convention
support and whitelist exceptions for command, event, and type
names.
The additions to QAPISchemaMember.check_clash() check whether
info['name'] is in the whitelist (the top-most entity name at
the point 'info' tracks), rather than self.owner (the type,
possibly implicit, that directly owns the member), because it
is easier to maintain the whitelist by the names actually in
the user's .json file, rather than worrying about the names
of implicit types.
Backports commit 893e1f2c5170d54316f1dcf3fefae679175622fc from qemu
Rather than using just an array of strings, make enum.values be
an array of the new QAPISchemaMember type, and add a helper
member_names() method to get back at the original list of names.
Likewise, creating an enum requires wrapping strings, via a new
QAPISchema._make_enum_members() method. The benefit of wrapping
enum members in a QAPISchemaMember Python object is that we now
share the existing code for C name clash detection (although the
code is not yet active until a later commit removes the earlier
ad hoc parser checks).
In a related change, the QAPISchemaMember._pretty_owner() method
needs to learn about one more implicit type name: the generated
enum associated with a simple union.
In the interest of keeping the changes of this patch local to one
file, the visitor interface still passes just a list of names
rather than the full list of QAPISchemaMember instances. We may
want to revisit this in the future, if the consistency with
visit_object_type() is worth it.
Backports commit 93bda4dd461358b4fc05dfd8e2d6419cdd574789 from qemu
We want to share some clash detection code between enum values
and object type members. To assist with that, split off part
of QAPISchemaObjectTypeMember into a new base class
QAPISchemaMember that tracks name, owner, and common clash
detection code; while the former keeps the additional fields
for type and optional flag.
Backports commit d44f9ac80c43e34b1522cde8829f0ab371f086ca from qemu
For less code, reflect the determined boolean value of an optional
visit back to the caller instead of making the caller read the
boolean after the fact.
The resulting generated code has the following diff:
|- visit_optional(v, &has_fdset_id, "fdset-id");
|- if (has_fdset_id) {
|+ if (visit_optional(v, &has_fdset_id, "fdset-id")) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Backports commit 29637a6ee913df8fcdf371426ee48956b945b618 from qemu
None of the visitor callbacks would set an error when testing
if an optional field was present; make this part of the interface
contract by eliminating the errp argument.
The resulting generated code has a nice diff:
|- visit_optional(v, &has_fdset_id, "fdset-id", &err);
|- if (err) {
|- goto out;
|- }
|+ visit_optional(v, &has_fdset_id, "fdset-id");
| if (has_fdset_id) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Backports commit 5cdc8831a795fb8452d7e34f644202fd724e122a 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
Now that alternates no longer use an implicit tag, we can
inline _make_implicit_tag() into its one caller,
_def_union_type().
No change to generated code.
Backports commit 9d3f3494c5b941774e2c3e7639332d53bbe6f8be from qemu
Previously, the generated code in qapi-types.c initialized all
enum lookup tables first, prior to any other definitions. But
there are no topological sorting requirements that mandate this
layout, so we can drop the QAPISchemaGenTypeVisitor._fwdefn
field and just generate all definitions in visitation order.
The generated code shows some churn due to reordering, but it
is still fairly straightforward to follow (all the deletions
occur in one hunk, and all the deleted lines are re-inserted
in the same order later in the same files, just spread across
multiple insertion points).
Backports commit 0b2e84ba774651656771ed697dee8825759dffa9 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
The name QType matches our CODING_STYLE conventions for type names
in CamelCase. It also matches the fact that we are already naming
all the enum members with a prefix of QTYPE, not QTYPE_CODE. And
doing the rename will also make it easier for the next patch to use
QAPI for providing the enum, which also wants CamelCase type names.
Backports commit 1310a3d3bd9301ff5a825287638cfab24c2c6689 from qemu
When munging enum values, the fact that we were passing the entire
prefix + value through camel_to_upper() meant that enum values
spelled with CamelCase could be turned into CAMEL_CASE. However,
this provides a potential collision (both OneTwo and One-Two would
munge into ONE_TWO) for enum types, when the same two names are
valid side-by-side as QAPI member names. By changing the generation
of enum constants to always be prefix + '_' + c_name(value,
False).upper(), and ensuring that there are no case collisions (in
the next patches), we no longer have to worry about names that
would be distinct as QAPI members but collide as variant tag names,
without having to think about what munging the heuristics in
camel_to_upper() will actually perform on an enum value.
Making the change will affect enums that did not follow coding
conventions, using 'CamelCase' rather than desired 'lower-case'.
Thankfully, there are only two culprits: InputButton and ErrorClass.
We already tweaked ErrorClass to make it an alias of QapiErrorClass,
where only the alias needs changing rather than the whole tree. So
the bulk of this change is modifying INPUT_BUTTON_WHEEL_UP to the
new INPUT_BUTTON_WHEELUP (and likewise for WHEELDOWN). That part
of this commit may later need reverting if we rename the enum
constants from 'WheelUp' to 'wheel-up' as part of moving
x-input-send-event to a stable interface; but at least we have
documentation bread crumbs in place to remind us (commit 513e7cd),
and it matches the fact that SDL constants are also spelled
SDL_BUTTON_WHEELUP.
Backports commit d20a580bc0eac9d489884f6d2ed28105880532b6 from qemu
Now that we no longer collide with an implicit _MAX enum member,
we no longer need to reject it in the ad hoc parser, and can
remove several tests that are no longer needed.
Backports commit 04e0639d4e77b6d55491d396c8aa13929ee8ed9a from qemu
Now that we guarantee the user doesn't have any enum values
beginning with a single underscore, we can use that for our
own purposes. Renaming ENUM_MAX to ENUM__MAX makes it obvious
that the sentinel is generated.
This patch was mostly generated by applying a temporary patch:
|diff --git a/scripts/qapi.py b/scripts/qapi.py
|index e6d014b..b862ec9 100644
|--- a/scripts/qapi.py
|+++ b/scripts/qapi.py
|@@ -1570,6 +1570,7 @@ const char *const %(c_name)s_lookup[] = {
| max_index = c_enum_const(name, 'MAX', prefix)
| ret += mcgen('''
| [%(max_index)s] = NULL,
|+// %(max_index)s
| };
| ''',
| max_index=max_index)
then running:
$ cat qapi-{types,event}.c tests/test-qapi-types.c |
sed -n 's,^// \(.*\)MAX,s|\1MAX|\1_MAX|g,p' > list
$ git grep -l _MAX | xargs sed -i -f list
The only things not generated are the changes in scripts/qapi.py.
Rejecting enum members named 'MAX' is now useless, and will be dropped
in the next patch.
Backports commit 7fb1cf1606c78c9d5b538f29176fd5a101726a9d from qemu
We already documented that qapi names should match specific
patterns (such as starting with a letter unless it was an enum
value or a downstream extension). Tighten that from a suggestion
into a hard requirement, which frees up names beginning with a
single underscore for qapi internal usage.
The tighter regex doesn't forbid everything insane that a user
could provide (for example, a user could name a type 'Foo-lookup'
to collide with the generated 'Foo_lookup[]' for an enum 'Foo'),
but does a good job at protecting the most obvious uses, and
also happens to reserve single leading underscore for later use.
The handling of enum values starting with a digit is tricky:
commit 9fb081e introduced a subtle bug by using c_name() on
a munged value, which would allow an enum to include the
member 'q-int' in spite of our reservation. Furthermore,
munging with a leading '_' would fail our tighter regex. So
fix it by only munging for leading digits (which are never
ticklish in c_name()) and by using a different prefix (I
picked 'D', although any letter should do).
Add new tests, reserved-member-underscore and reserved-enum-q,
to demonstrate the tighter checking.
Backports commit 59a92feedc6927e0e1ff87fdaccfb4dd42ad4c84 from qemu
The method c_name() is supposed to do two different actions: munge
'-' into '_', and add a 'q_' prefix to ticklish names. But it did
these steps out of order, making it possible to submit input that
is not ticklish until after munging, where the output then lacked
the desired prefix.
The failure is exposed easily if you have a compiler that recognizes
C11 keywords, and try to name a member '_Thread-local', as it would
result in trying to compile the declaration 'uint64_t _Thread_local;'
which is not valid. However, this name violates our conventions
(ultimately, want to enforce that no qapi names start with single
underscore), so the test is slightly weaker by instead testing
'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where
wchar_t is only a typedef) but would fail with a C++ compiler (where
it is a keyword).
Fix things by reversing the order of actions within c_name().
Backports commit c43567c12042cf401b039bfc94a5f85e1cc1e796 from qemu
Detect attempts to declare two object members that would result
in the same C member name, by keying the 'seen' dictionary off
of the C name rather than the qapi name. It also requires passing
info through the check_clash() methods.
This addresses a TODO and fixes the previously-broken
args-name-clash test. The resulting error message demonstrates
the utility of the .describe() method added previously. No change
to generated code.
Backports commit 27b60ab93bd1d5d8c85f009aac7a97ffd2c53c86 from qemu
Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods. But to
report an error message about an incorrect semantic use of a
member of an object type, it helps to know which type, command,
or event owns the member. In particular, when a member is
inherited from a base type, it is desirable to associate the
member name with the base type (and not the type calling
member.check()).
Rather than packing additional information into the seen array
passed to each member.check() (as in seen[m.name] = {'member':m,
'owner':type}), it is easier to have each member track the name
of the owner type in the first place (keeping things simpler
with the existing seen[m.name] = m). The new member.owner field
is set via a new set_owner() method, called when registering
the members and variants arrays with an object or variant type.
Track only a name, and not the actual type object, to avoid
creating a circular python reference chain.
Note that Variants.set_owner() method does not set the owner
for the tag_member field; this field is set earlier either as
part of an object's non-variant members, or explicitly by
alternates.
The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information. For example, given the qapi:
{ 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
arg_type.members[0].describe()
will see "'string' (parameter of foo)".
To make the human-readable name of implicit types work without
duplicating efforts, the describe() method has to reverse the
name of implicit types, via the helper _pretty_owner().
No change to generated code.
Backports commit 88d4ef8b5cbf9d3336564b1d3ac7a91cbe4aee0e from qemu
Checking that a given QAPISchemaObjectTypeVariant.name is a
member of the corresponding QAPISchemaEnumType of the owning
QAPISchemaObjectTypeVariants.tag_member ensures that there are
no collisions in the generated C union for those tag values
(since the enum itself should have no collisions).
However, ever since its introduction in f51d8c3d, this was the
only additional action of of Variant.check(), beyond calling
the superclass Member.check(). This forces a difference in
.check() signatures, just to pass the enum type down.
Simplify things by instead doing the tag name check as part of
Variants.check(), at which point we can rely on inheritance
instead of overriding Variant.check().
Backports commit 10565ca92a8d3f8a34559329acfbdb25a791b594 from qemu
Consolidate two common sequences of clash detection into a
new QAPISchemaObjectType.check_clash() helper method.
No change to generated code.
Backports commit c2183d2e62b6d9d66f80bc0bcf4fc7ec3c5d76d4 from qemu
Right now, our ad hoc parser ensures that we cannot have a
flat union that introduces any members that would clash with
non-variant members inherited from the union's base type (see
flat-union-clash-member.json). We want QAPISchemaObjectType.check()
to make the same check, so we can later reduce some of the ad
hoc checks.
We already have a map 'seen' of all non-variant members. We
still need to check for collisions between each variant type's
members and the non-variant ones.
To know the variant type's members, we need to call
variant.type.check(). This also detects when a type contains
itself in a variant, exactly like the existing base.check()
detects when a type contains itself as a base. (Except that
we currently forbid anything but a struct as the type of a
variant, so we can't actually trigger this type of loop yet.)
Slight complication: an alternate's variant can have arbitrary
type, but only an object type's check() may be called outside
QAPISchema.check(). We could either skip the call for variants
of alternates, or skip it for non-object types. For now, do
the latter, because it's easier.
Then we call each variant member's check_clash() with the
appropriate 'seen' map. Since members of different variants
can't clash, we have to clone a fresh seen for each variant.
Wrap this in a new helper method
QAPISchemaObjectTypeVariants.check_clash().
Note that cloning 'seen' inside .check_clash() resembles
the one we just removed from .check() in 'qapi: Drop
obsolete tag value collision assertions'; the difference here is
that we are now checking for clashes among the qapi members of
the variant type, rather than for a single clash with the variant
tag name itself.
Note that, by construction, collisions can't actually happen for
simple unions: each variant's type is a wrapper with a single
member 'data', which will never collide with the only non-variant
member 'type'.
For alternates, there's nothing for a variant object type's
members to clash with, and therefore no need to call the new
variants.check_clash().
No change to generated code.
Backports commit b807a1e1e3796adaf3ece2f7b69ea5ee28468ff4 from qemu
Reduce the ugly flat union / simple union conditional by doing just
the essential work here, namely setting self.tag_member.
Move the rest to callers.
Backports commit 14ff84619c6bb9b729dbf8b127c1e4c56ed8c500 from qemu
While there, stick in a TODO change key of seen from QAPI name to C
name. Can't do it right away, because it would fail the assertion for
tests/qapi-schema/args-has-clash.json.
Backports commit 577de12d22aba55f31fd68c5724411eb8592a4ca from qemu
This hunk
@@ -964,6 +965,7 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
+ assert c_name(m.name) not in seen
seen[m.name] = m
for m in self.local_members:
m.check(schema, members, seen)
is plainly broken.
Asserting the members inherited from base don't clash is somewhat
redundant, because self.base.check() just checked that. But it
doesn't hurt.
The idea to use c_name(m.name) instead of m.name for collision
checking is sound, because we need to catch clashes between the m.name
and between the c_name(m.name), and when two m.name clash, then their
c_name() also clash.
However, using c_name(m.name) instead of m.name in one of several
places doesn't work. See the very next line.
Keep the assertion, but drop the c_name() for now. A future commit
will bring it back.
Backports commit 08683353fc46b5d462199c9e8cff6f6c67f20f65 from qemu
QAPISchemaObjectTypeVariants.check() parameter members and
QAPISchemaObjectTypeVariant.check() parameter seen are no longer used,
drop them.
Backports commit cdc5fa37eda2896d2b08f9215c963256eb859d3b from qemu
QAPISchemaObjectTypeMember.check() currently does four things:
1. Compute self.type
2. Accumulate members in all_members
Only one caller cares: QAPISchemaObjectType.check() uses it to
compute self.members. The other callers pass a throw-away
accumulator.
3. Accumulate a map from names to members in seen
Only one caller cares: QAPISchemaObjectType.check() uses it to
compute its local variable seen, for self.variants.check(), which
uses it to compute self.variants.tag_member from
self.variants.tag_name. The other callers pass a throw-away
accumulator.
4. Check for collisions
This piggybacks on 3: before adding a new entry, we assert it's new.
Only one caller cares: QAPISchemaObjectType.check() uses it to
assert non-variant members don't clash.
Simplify QAPISchemaObjectType.check(): move 2.-4. to
QAPISchemaObjectType.check(), and drop parameters all_members and
seen.
Backports commit e564e2dd5963a75f32bbb90ac8181ba9dca2f1aa from qemu
Union tag values can't clash with member names in generated C anymore
since commit e4ba22b, but QAPISchemaObjectTypeVariants.check() still
asserts they don't. Drop it.
Backports commit fff5f231d5f96e8521761efcd35a12479594059a from qemu
