Add forgotten zero-extension in the TARGET_X86_64, !CODE64, ss32 case;
use this new function to implement gen_string_movl_A0_EDI,
gen_string_movl_A0_ESI, gen_add_A0_ds_seg.
Backports commit ca2f29f555805d07fb0b9ebfbbfc4e3656530977 from qemu
This condition is true in the common case, so we can cut out the body of
the function. In addition, this makes it easier for the compiler to do
at least partial inlining, even if it decides that fully inlining the
function is unreasonable.
Backports commit 8bafcb21643a39a5b29109f8bd5ee5a6f0f6850b from qemu
Similar to error_abort, but doesn't report where the error was
created, and terminates the process with exit(1) rather than abort().
Backports commit a29a37b994ca3c5a1d39fa0e8934f7e0f2cf57ef from qemu
Don't claim error_report_err() always reports to stderr. It actually
reports to the current monitor when we have one.
Clarify intended use of error_abort and error_fatal.
Backports commit 10303f04b98efa76e638b9ae4632688f56f088fc 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
The previous commit documented an inconsistency in how we are
using the stack of qmp-output-visitor. Normally, pushing a
single top-level object puts the object on the stack twice:
once as the root, and once as the current container being
appended to; but popping that struct only pops once. However,
qmp_ouput_add() was trying to either set up the added object
as the new root (works if you parse two top-level scalars in a
row: the second replaces the first as the root) or as a member
of the current container (works as long as you have an open
container on the stack; but if you have popped the first
top-level container, it then resolves to the root and still
tries to add into that existing container).
Fix the stupidity by not tracking two separate things in the
stack. Drop the now-useless qmp_output_first() and
qmp_output_last() while at it.
Saved for a later patch: we still are rather sloppy in that
qmp_output_get_object() can be called in the middle of a parse,
rather than requiring that a visit is complete.
Backports commit 455ba08afde784466420449d01c6458f88349d55 from qemu
Commit 6c2f9a15 ensured that we would not return NULL when the
caller used an output visitor but had nothing to visit. But
in doing so, it added a FIXME about a reference count leak
that could abort qemu in the (unlikely) case of SIZE_MAX such
visits (more plausible on 32-bit). (Although that commit
suggested we might fix it in time for 2.5, we ran out of time;
fortunately, it is unlikely enough to bite that it was not
worth worrying about during the 2.5 release.)
This fixes things by documenting the internal contracts, and
explaining why the internal function can return NULL and only
the public facing interface needs to worry about qnull(),
thus avoiding over-referencing the qnull_ global object.
It does not, however, fix the stupidity of the stack mixing
up two separate pieces of information; add a FIXME to explain
that issue, which will be fixed shortly in a future patch.
Backports commit a86156401559cb4401cf9ecc704faeab6fc8bb19 from qemu
The only way that qmp_input_pop() will set errp is if a dictionary
was the most recent thing pushed. Since we don't have any
push(struct)/pop(list) or push(list)/pop(struct) mismatches (such
a mismatch is a programming bug), we therefore cannot set errp
inside qmp_input_end_list(). Make this obvious by
using &error_abort. A later patch will then remove the errp
parameter of qmp_input_pop(), but that will first require the
larger task of splitting visit_end_struct().
Backports commit bdd8e6b5d8a9def83d491a3f41c10424fc366258 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
As explained in the previous patches, matching argument order of
'name, &value' to JSON's "name":value makes sense. However,
while the last two patches were easy with Coccinelle, I ended up
doing this one all by hand. Now all the visitor callbacks match
the main interface.
The compiler is able to enforce that all clients match the changed
interface in visitor-impl.h, even where two pointers are being
swapped, because only one of the two pointers is const (if that
were not the case, then C's looseness on treating 'char *' like
'void *' would have made review a bit harder).
Backports commit 0b2a0d6bb2446060944061e53e87d0c7addede79 from qemu
Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but no visitor has supplied any of the
callbacks for sizes less than 64 bits. In other words, the
generic implementation based on using type_[u]int64() followed
by bounds-checking works just fine. In the interest of
simplicity, it's easier to make the visitor callback interface
not have to worry about the other sizes.
Adding some helper functions minimizes the boilerplate required
to correct FIXMEs added earlier with regards to questionable
reuse of errp, particularly now that we can guarantee from a
single file audit that value is unchanged if an error is set.
Backports commit 04e070d217b4414f1f91aa8ad25fc0ae7ca0be93 from qemu
Similar to the previous patch, it's nice to have all functions
in the tree that involve a visitor and a name for conversion to
or from QAPI to consistently stick the 'name' parameter next
to the Visitor parameter.
Done by manually changing include/qom/object.h and qom/object.c,
then running this Coccinelle script and touching up the fallout
(Coccinelle insisted on adding some trailing whitespace).
@ rule1 @
identifier fn;
typedef Object, Visitor, Error;
identifier obj, v, opaque, name, errp;
@@
void fn
- (Object *obj, Visitor *v, void *opaque, const char *name,
+ (Object *obj, Visitor *v, const char *name, void *opaque,
Error **errp) { ... }
@@
identifier rule1.fn;
expression obj, v, opaque, name, errp;
@@
fn(obj, v,
- opaque, name,
+ name, opaque,
errp)
Backports commit d7bce9999df85c56c8cb1fcffd944d51bff8ff48 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
No need to repeat 'struct Visitor' when we already have it in
typedefs.h. Omitting the redundant 'struct' also makes a later
patch easier to search for all object property callbacks that
are associated with a Visitor.
Backports commit 4fa45492c3387c0fa51e8e81160ac9a7814f44a2 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
The QObject hierarchy is small enough, and unlikely to grow further
(since we only use it to map to JSON and already cover all JSON
types), that we can simplify things by not tracking a separate
vtable, but just inline the code element of the vtable QType
directly into QObject (renamed to type), and track a separate array
of destroy functions. We can drop qnull_destroy_obj() in the
process.
The remaining QObject subclasses must export their destructor.
This also has the nice benefit of moving the typename 'QType'
out of the way, so that the next patch can repurpose it for a
nicer name for 'qtype_code'.
The various objects are still the same size (so no change in cache
line pressure), but now have less indirection (although I didn't
bother benchmarking to see if there is a noticeable speedup, as
we don't have hard evidence that this was in a performance hotspot
in the first place).
A future patch could drop the refcnt size to 32 bits for a smaller
struct on 64-bit architectures, if desired (we have limits on the
largest JSON that we are willing to parse, and will probably never
need to take full advantage of a 64-bit refcnt).
Backports commit 55e1819c509b3d9c10a54678b9c585bbda13889e from qemu
I'm going to fix the JSON parser to recognize null. The obvious
representation of JSON null as (QObject *)NULL doesn't work, because
the parser already uses it as an error value. Perhaps we should
change it to free NULL for null, but that's more than I can do right
now. Create a special null QObject instead.
The existing QDict, QList, and QString all represent something that
is a pointer in C and could therefore be associated with NULL. But
right now, all three of these sub-types are always non-null once
created, so the new null sentinel object is intentionally unrelated
to them.
Backports commit 481b002cc81ed7fc7b06e32e9d4d495d81739d14 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
The qapi enum ErrorClass is unusual that it uses 'CamelCase' names,
contrary to our documented convention of preferring 'lower-case'.
However, this enum is entrenched in the API; we cannot change
what strings QMP outputs. Meanwhile, we want to simplify how
c_enum_const() is used to generate enum constants, by moving away
from the heuristics of camel_to_upper() to a more straightforward
c_name(N).upper() - but doing so will rename all of the ErrorClass
constants and cause churn to all client files, where the new names
are aesthetically less pleasing (ERROR_CLASS_DEVICENOTFOUND looks
like we can't make up our minds on whether to break between words).
So as always in computer science, solve the problem by some more
indirection: rename the qapi type to QapiErrorClass, and add a
new enum ErrorClass in error.h whose members are aliases of the
qapi type, but with the spelling expected elsewhere in the tree.
Then, when c_enum_const() changes the munging, we only have to
adjust the one alias spot.
Backports commit f22a28b898322c01b0463a8b7ec551d72bc61a5b 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
Commit cbc95538 removed unused start_handle() and end_handle(),
but forgot to remove their declarations.
Backports commit 7549457200ec3871ee827765f4d3bbc8d903b2ec 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
Simplify gen_struct_fields() back to a single iteration over a
list of fields (like it was prior to commit f87ab7f9), by moving
the generated comments to gen_object(). Then, inline
gen_struct_field() into its only caller.
Backports commit 7d9586f900a9043025866f84c096b1842b0bbbf6 from qemu
These two methods are now close enough that we can finally merge
them, relying on the fact that simple unions now provide a
reasonable local_members. Change gen_struct() to gen_object()
that handles all forms of QAPISchemaObjectType, and rename and
shrink gen_union() to gen_variants() to handle the portion of
gen_object() needed when variants are present.
gen_struct_fields() now has a single caller, so it no longer
needs an optional parameter; however, I did not choose to inline
it into the caller.
No difference to generated code.
Backports commit 570cd8d1194cf68f7c9948971e52e47f20855a77 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
Rather than having all callers pass a name, type, and optional
flag, have them instead pass a QAPISchemaObjectTypeMember which
already has all that information.
No change to generated code.
Backports commit 32bc6879beea0b0cac6196cb15a71d206401e96d from qemu
Now that we have separated union tag values from colliding with
non-variant C names, by naming the union 'u', we should reserve
this name for our use. Note that we want to forbid 'u' even in
a struct with no variants, because it is possible for a future
qemu release to extend QMP in a backwards-compatible manner while
converting from a struct to a flat union. Fortunately, no
existing clients were using this member name. If we ever find
the need for QMP to have a member 'u', we could at that time
relax things, perhaps by having c_name() munge the QMP member to
'q_u'.
Note that we cannot forbid 'u' everywhere (by adding the
rejection code to check_name()), because the existing QKeyCode
enum already uses it; therefore we only reserve it as a struct
type member name.
Backports commit 5e59baf90a72cd25d38a3134edc029f4f022da74 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.
This patch is the back end for a series that converts to a
saner qapi union layout. Now that all clients have been
converted to use 'type' and 'obj->u.value', we can drop the
temporary parallel support for 'kind' and 'obj->value'.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
this is the overall effect, when compared to the state before
this series of patches:
| struct Foo {
|- FooKind kind;
|- union { /* union tag is @kind */
|+ FooKind type;
|+ union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|- };
|+ } u;
| };
The testsuite still contains some examples of artificial restrictions
(see flat-union-clash-type.json, for example) that are no longer
technically necessary, now that there is no longer a collision between
enum tag values and non-variant member names; but fixing this will be
done in later patches, in part because some further changes are required
to keep QAPISchema*.check() from asserting. Also, a later patch will
add a reservation for the member name 'u' to avoid a collision between a
user's non-variant names and our internal choice of C union name.
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Backports commit e4ba22b31943ab02373359555bd7bcd66442632f 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
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.
This patch is the front end for a series that converts to a
saner qapi union layout. By the end of the series, we will no
longer have the type/kind mismatch, and all tag values will be
under a named union, which requires clients to access
'obj->u.value' instead of 'obj->value'. But since the
conversion touches a number of files, it is easiest if we
temporarily support BOTH layouts simultaneously.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
make the following changes in generated qapi-types.h:
| struct Foo {
|- FooKind kind;
|- union { /* union tag is @kind */
|+ union {
|+ FooKind kind;
|+ FooKind type;
|+ };
|+ union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|+ union { /* union tag is @type */
|+ void *data;
|+ int64_t a;
|+ bool b;
|+ } u;
| };
| };
Flat unions do not need the anonymous union for the tag member,
as we already fixed that to use the member name instead of 'kind'
back in commit 0f61af3e.
One additional change is needed in qapi.py: check_union() now
needs to check for collisions with 'type' in addition to those
with 'kind'.
Later, when the conversions are complete, we will remove the
duplication hacks, and also drop the check_union() restrictions.
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Backports commit f51d8fab44b231aa299d8de24cfdf9ba41ef4a21 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
A previous patch (commit 1e6c1616) made it possible to
directly cast from a qapi flat union type to its base type.
However, it requires the use of a C cast, which turns off
compiler type-safety checks. Fortunately, no such casts
exist, just yet.
Regardless, add inline type-safe wrappers named
qapi_FOO_base() for any union type FOO that has a base,
which can be used for a safer upcast, and enhance the
testsuite to cover the new functionality.
A future patch will extend the upcast support to structs,
where such conversions do exist already.
Note that C makes const-correct upcasts annoying because
it lacks overloads; these functions cast away const so that
they can accept user pointers whether const or not, and the
result in turn can be assigned to normal or const pointers.
Alternatively, this could have been done with macros, but
type-safe macros are hairy, and not worthwhile here.
This patch just adds upcasts. None of our code needed to
downcast from a base qapi class to a child. Also, in the
case of grandchildren (such as BlockdevOptionsQcow2), the
caller will need to call two functions to get to the inner
base (although it wouldn't be too hard to generate a
qapi_FOO_base_base() if desired). If a user changes qapi
to alter the base class hierarchy, such as going from
'A -> C' to 'A -> B -> C', it will change the type of
'qapi_C_base()', and the compiler will point out the places
that are affected by the new base.
One alternative was proposed, but was deemed too ugly to use
in practice: the generators could output redundant
information using anonymous types:
| struct Child {
| union {
| struct {
| Type1 parent_member1;
| Type2 parent_member2;
| };
| Parent base;
| };
| };
With that ugly proposal, for a given qapi type, obj->member
and obj->base.member would refer to the same storage; allowing
convenience in working with members without needing 'base.'
allowing typesafe upcast without needing a C cast by accessing
'&obj->base', and allowing downcasts from the parent back to
the child possible through container_of(obj, Child, base).
Backports commit 30594fe1cd4355626e73b80645428105d0df3cf6 from qemu
Move code from gen_union() into gen_struct_fields() in order for
a later patch to share code when enumerating inherited fields
for struct types.
No change to generated code.
Backports commit f87ab7f9bd956250c48b5c6e9b607b537fd21543 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
c_name() produces names starting with 'q_' when protecting a
dictionary member name that would fail to directly compile, but
in doing so can cause clashes with any member name already
beginning with 'q-' or 'q_'. Likewise, we create a C name 'has_'
for any optional member that can clash with any member name
beginning with 'has-' or 'has_'.
Technically, rather than blindly reserving the namespace,
we could try to complain about user names only when an actual
collision occurs, or even teach c_name() how to munge names
to avoid collisions. But it is not trivial, especially when
collisions can occur across multiple types (such as via
inheritance or flat unions). Besides, no existing .json
files are trying to use these names. So it's easier to just
outright forbid the potential for collision. We can always
relax things in the future if a real need arises for QMP to
express member names that have been forbidden here.
'has_' only has to be reserved for struct/union member names,
while 'q_' is reserved everywhere (matching the fact that
only members can be optional, while we use c_name() for munging
both members and entities). Note that we could relax 'q_'
restrictions on entities independently from member names; for
example, c_name('qmp_' + 'unix') would result in a different
function name than our current 'qmp_' + c_name('unix').
Update and add tests to cover the new error messages.
Backports commit 9fb081e0b98409556d023c7193eeb68947cd1211 from qemu
Type names ending in 'List' can clash with qapi list types in
generated C. We don't currently use such names. It is easier to
outlaw them now than to worry about how to resolve such a clash
in the future. For precedence, see commit 4dc2e69, which did the
same for names ending in 'Kind' versus implicit enum types for
qapi unions.
Update the testsuite to match.
Backports commit 255960dd374d4497d6ea537305f1b0d8a3433789 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
Rather than slicing the end of a string, we can use python's
endswith(). And rather than creating a set of characters,
we can search for a character within a string.
Backports commit 8712fa5333ad348da20034b717dd814219d1ec11 from qemu
A future patch will move some error checking from the parser
to the various QAPISchema*.check() methods, which run only
after parsing completes. It will thus be possible to create
a python instance representing an implicit QAPI type that
parses fine but will fail validation during check(). Since
all errors have to have an associated 'info' location, we
need a location to be associated with those implicit types.
The intuitive info to use is the location of the enclosing
entity that caused the creation of the implicit type.
Note that we do not anticipate builtin types being used in
an error message (as they are not part of the user's QAPI
input, the user can't cause a semantic error in their
behavior), so we exempt those types from requiring info, by
setting a flag to track the completion of _def_predefineds(),
and tracking that flag in _def_entity().
No change to the generated code.
Backports commit 99df5289d8c7ebf373c3570d8fba3f3a73360281 from qemu
For simple unions, we were creating the implicit 'type' tag
member during the QAPISchemaObjectTypeVariants constructor.
This is different from every other implicit QAPISchemaEntity
object, which get created by QAPISchema methods. Hoist the
creation to the caller (renaming _make_tag_enum() to
_make_implicit_tag()), and pass the entity rather than the
string name, so that we have the nice property that no
entities are created as a side effect within a different
entity. A later patch will then have an easier time of
associating location info with each entity creation.
No change to generated code.
Backports commit 46292ba75c515baf733df18644052b2ce9492728 from qemu
Commit ac88219a had several TODO markers about whether we needed
to automatically create the corresponding array type alongside
any other type. It turns out that most of the time, we don't!
There are a few exceptions: 1) We have a few situations where we
use an array type in internal code but do not expose that type
through QMP; fix it by declaring a dummy type that forces the
generator to see that we want to use the array type.
2) The builtin arrays (such as intList for QAPI ['int']) must
always be generated, because of the way our QAPI_TYPES_BUILTIN
compile guard works: we have situations (at the very least
tests/test-qmp-output-visitor.c) that include both top-level
"qapi-types.h" (via "error.h") and a secondary
"test-qapi-types.h". If we were to only emit the builtin types
when used locally, then the first .h file would not include all
types, but the second .h does not declare anything at all because
the first .h set QAPI_TYPES_BUILTIN, and we would end up with
compilation error due to things like unknown type 'int8List'.
Actually, we may need to revisit how we do type guards, and
change from a single QAPI_TYPES_BUILTIN over to a different
usage pattern that does one #ifdef per qapi type - right now,
the only types that are declared multiple times between two qapi
.json files for inclusion by a single .c file happen to be the
builtin arrays. But now that we have QAPI 'include' statements,
it is logical to assume that we will soon reach a point where
we want to reuse non-builtin types (yes, I'm thinking about what
it will take to add introspection to QGA, where we will want to
reuse the SchemaInfo type and friends). One #ifdef per type
will help ensure that generating the same qapi type into more
than one qapi-types.h won't cause collisions when both are
included in the same .c file; but we also have to solve how to
avoid creating duplicate qapi-types.c entry points. So that
is a problem left for another day.
Generated code for qapi-types and qapi-visit is drastically
reduced; less than a third of the arrays that were blindly
created were actually needed (a quick grep shows we dropped
from 219 to 69 *List types), and the .o files lost more than
30% of their bulk. [For best results, diff the generated
files with 'git diff --patience --no-index pre post'.]
Interestingly, the introspection output is unchanged - this is
because we already cull all types that are not indirectly
reachable from a command or event, so introspection was already
using only a subset of array types. The subset of types
introspected is now a much larger percentage of the overall set
of array types emitted in qapi-types.h (since the larger set
shrunk), but still not 100% (evidence that the array types
emitted for our new Dummy structs, and the new struct itself,
don't affect QMP).
Backports commit 9f08c8ec73878122ad4b061ed334f0437afaaa32 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
The next few patches will start migrating error checking from
ad hoc parse methods into the QAPISchema*.check() methods. But
for an error message to display, we first have to fix the
overall 'try' to catch those errors. We also want to enable a
few more assertions, such as making sure every attempt to
raise a semantic error is passed a valid location info, or that
various preconditions hold.
The general approach for moving error checking will then be to
relax an assertion into an if that raises an exception if the
condition does not hold, and removing the counterpart ad hoc
check done during the parse phase.
Backports commit 7618b91ff80ec42b84b29be24d8ef53ddb377110 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
Since we have consolidated all generated code to use 'err' as
the name of the local variable for error detection, we can
simplify the decision on whether to skip error detection (useful
for deallocation paths) to be a boolean.
Backports commit 18bdbc3ac8b477e160d56aa6ecd6942495ce44d0 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
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 consistently names the local error variable 'err' rather
than 'local_err'.
No change in semantics to the generated code.
Backports commit 2a0f50e8d973b01eda4c63bac4a5c79ea0f584ef from qemu
Rather than open-code the check for a valid base type, we
should reuse the common functionality. This allows for
consistent error messages, and also makes it easier for a
later patch to turn on support for inline anonymous base
structures.
Test flat-union-inline is updated to test only one feature
(anonymous branch dictionaries), which can be implemented
independently (test flat-union-bad-base already covers the
idea of an anonymous base dictionary).
Backports commit 376863ef4895ae709aadb6f26365a5973310ef09 from qemu
The previous commit added two tests that triggered an assertion
failure. It's fairly straightforward to avoid the failure by
just outright forbidding the collision between a union's tag
values and its discriminator name (including the implicit name
'kind' supplied for simple unions [*]). Ultimately, we'd like
to move the collision detection into QAPISchema*.check(), but
for now it is easier just to enhance the existing checks.
[*] Of course, down the road, we have plans to rename the simple
union tag name to 'type' to match the QMP wire name, but the
idea of the collision will still be present even then.
Technically, we could avoid the collision by naming the C union
members representing each enum value as '_case_value' rather
than 'value'; but until we have an actual qapi client (and not
just our testsuite) that has a legitimate reason to match a
case label to the name of a QMP key and needs the name munging
to satisfy the compiler, it's easier to just reject the qapi
as invalid.
Backports commit 7b2a5c2f9a52c4a08630fa741052f03fe5d3cc8a from qemu
Silence pep8, and make pylint a bit happier. Just style cleanups,
plus killing a useless comment in camel_to_upper(); no semantic
changes.
Backports commit 437db2549be383e52acad6cd4bf2862e98fdfc93 from qemu
pylint recommends that every exception class should explicitly
invoke the superclass __init__, even though things seem to work
fine without it.
Backports commit 59b00542659c8947f9d4e8c28d2d528ab3ab61a5 from qemu
Use of '"...%s" % include' to print non-strings can lead to
ugly messages, such as this (if the .json change is applied
without the qapi.py change):
Expected a file name (string), got: OrderedDict()
Better is to just omit the actual non-string value in the
message.
Backports commit 7408fb67c0f9403f6e40aecf97cf798fc14e2cd8 from qemu
qapi/introspect.json defines the introspection schema. It's designed
for QMP introspection, but should do for similar uses, such as QGA.
The introspection schema does not reflect all the rules and
restrictions that apply to QAPI schemata. A valid QAPI schema has an
introspection value conforming to the introspection schema, but the
converse is not true.
Introspection lowers away a number of schema details, and makes
implicit things explicit:
* The built-in types are declared with their JSON type.
All integer types are mapped to 'int', because how many bits we use
internally is an implementation detail. It could be pressed into
external interface service as very approximate range information,
but that's a bad idea. If we need range information, we better do
it properly.
* Implicit type definitions are made explicit, and given
auto-generated names:
- Array types, named by appending "List" to the name of their
element type, like in generated C.
- The enumeration types implicitly defined by simple union types,
named by appending "Kind" to the name of their simple union type,
like in generated C.
- Types that don't occur in generated C. Their names start with ':'
so they don't clash with the user's names.
* All type references are by name.
* The struct and union types are generalized into an object type.
* Base types are flattened.
* Commands take a single argument and return a single result.
Dictionary argument or list result is an implicit type definition.
The empty object type is used when a command takes no arguments or
produces no results.
The argument is always of object type, but the introspection schema
doesn't reflect that.
The 'gen': false directive is omitted as implementation detail.
The 'success-response' directive is omitted as well for now, even
though it's not an implementation detail, because it's not used by
QMP.
* Events carry a single data value.
Implicit type definition and empty object type use, just like for
commands.
The value is of object type, but the introspection schema doesn't
reflect that.
* Types not used by commands or events are omitted.
Indirect use counts as use.
* Optional members have a default, which can only be null right now
Instead of a mandatory "optional" flag, we have an optional default.
No default means mandatory, default null means optional without
default value. Non-null is available for optional with default
(possible future extension).
* Clients should *not* look up types by name, because type names are
not ABI. Look up the command or event you're interested in, then
follow the references.
TODO Should we hide the type names to eliminate the temptation?
New generator scripts/qapi-introspect.py computes an introspection
value for its input, and generates a C variable holding it.
It can generate awfully long lines. Marked TODO.
A new test-qmp-input-visitor test case feeds its result for both
tests/qapi-schema/qapi-schema-test.json and qapi-schema.json to a
QmpInputVisitor to verify it actually conforms to the schema.
New QMP command query-qmp-schema takes its return value from that
variable. Its reply is some 85KiBytes for me right now.
If this turns out to be too much, we have a couple of options:
* We can use shorter names in the JSON. Not the QMP style.
* Optionally return the sub-schema for commands and events given as
arguments.
Right now qmp_query_schema() sends the string literal computed by
qmp-introspect.py. To compute sub-schema at run time, we'd have to
duplicate parts of qapi-introspect.py in C. Unattractive.
* Let clients cache the output of query-qmp-schema.
It changes only on QEMU upgrades, i.e. rarely. Provide a command
query-qmp-schema-hash. Clients can have a cache indexed by hash,
and re-query the schema only when they don't have it cached. Even
simpler: put the hash in the QMP greeting.
Backports commit 39a181581650f4d50f4445bc6276d9716cece050 from qemu
With the previous commit, the generated marshalers just work, and save
us a bit of handwritten code.
Backports commit 6eb3937e9b20319e1c4f4d53e906fda8f5ccda10 from qemu
It's first class, because unlike '**', it actually works, i.e. doesn't
require 'gen': false.
'**' will go away next.
Backports commit 28770e057f265a4e70bcbdfc2447cce7b5f2dc19 from qemu
