The next commit is going to make alternate members of type 'str'
conflict with other scalar types. Would break a few test cases that
don't actually require 'str'. Flip them from 'str' to 'bool' or
'EnumOne'.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1495471335-23707-4-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
This reverts commit 3313b61's changes to tests/qapi-schema/, except
for tests/qapi-schema/doc-*.
We could keep some of these doc comments to serve as positive test
cases. However, they don't actually add to what we get from doc
comment use in actual schemas, as we we don't test output matches
expectations, and don't systematically cover doc comment features.
Proper positive test coverage would be nice.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <1489582656-31133-4-git-send-email-armbru@redhat.com>
As the name suggests, the qapi2texi script converts JSON QAPI
description into a texi file suitable for different target
formats (info/man/txt/pdf/html...).
It parses the following kind of blocks:
Free-form:
##
# = Section
# == Subsection
#
# Some text foo with *emphasis*
# 1. with a list
# 2. like that
#
# And some code:
# | $ echo foo
# | -> do this
# | <- get that
#
##
Symbol description:
##
# @symbol:
#
# Symbol body ditto ergo sum. Foo bar
# baz ding.
#
# @param1: the frob to frobnicate
# @param2: #optional how hard to frobnicate
#
# Returns: the frobnicated frob.
# If frob isn't frobnicatable, GenericError.
#
# Since: version
# Notes: notes, comments can have
# - itemized list
# - like this
#
# Example:
#
# -> { "execute": "quit" }
# <- { "return": {} }
#
##
That's roughly following the following EBNF grammar:
api_comment = "##\n" comment "##\n"
comment = freeform_comment | symbol_comment
freeform_comment = { "# " text "\n" | "#\n" }
symbol_comment = "# @" name ":\n" { member | tag_section | freeform_comment }
member = "# @" name ':' [ text ] "\n" freeform_comment
tag_section = "# " ( "Returns:", "Since:", "Note:", "Notes:", "Example:", "Examples:" ) [ text ] "\n" freeform_comment
text = free text with markup
Note that the grammar is ambiguous: a line "# @foo:\n" can be parsed
both as freeform_comment and as symbol_comment. The actual parser
recognizes symbol_comment.
See docs/qapi-code-gen.txt for more details.
Deficiencies and limitations:
- the generated QMP documentation includes internal types
- union type support is lacking
- type information is lacking in generated documentation
- doc comment error message positions are imprecise, they point
to the beginning of the comment.
- a few minor issues, all marked TODO/FIXME in the code
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20170113144135.5150-16-marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[test-qapi.py tweaked to avoid trailing empty lines in .out]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Turn on the ability to pass command and event arguments in
a single boxed parameter, which must name a non-empty type
(although the type can be a struct with all optional members).
For structs, it makes it possible to pass a single qapi type
instead of a breakout of all struct members (useful if the
arguments are already in a struct or if the number of members
is large); for other complex types, it is now possible to use
a union or alternate as the data for a command or event.
The empty type may be technically feasible if needed down the
road, but it's easier to forbid it now and relax things to allow
it later, than it is to allow it now and have to special case
how the generated 'q_empty' type is handled (see commit 7ce106a9
for reasons why nothing is generated for the empty type). An
alternate type is never considered empty, but now that a boxed
type can be either an object or an alternate, we have to provide
a trivial QAPISchemaAlternateType.is_empty(). The new call to
arg_type.is_empty() during QAPISchemaCommand.check() requires
that we first check the type in question; but there is no chance
of introducing a cycle since objects do not refer back to commands.
We still have a split in syntax checking between ad-hoc parsing
up front (merely validates that 'boxed' has a sane value) and
during .check() methods (if 'boxed' is set, then 'data' must name
a non-empty user-defined type).
Generated code is unchanged, as long as no client uses the
new feature.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1468468228-27827-10-git-send-email-eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Test files renamed to *-boxed-*]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
The next patch will add support for passing a qapi union type
as the 'data' of a command. But to do that, the user function
for implementing the command, as called by the generated
marshal command, must take the corresponding C struct as a
single boxed pointer, rather than a breakdown into one
parameter per member. Even without a union, being able to use
a C struct rather than a list of parameters can make it much
easier to handle coding with QAPI.
This patch adds the internal plumbing of a 'boxed' flag
associated with each command and event. In several cases,
this means adding indentation, with one new dead branch and
the remaining branch being the original code more deeply
nested; this was done so that the new implementation in the
next patch is easier to review without also being mixed with
indentation changes.
For this patch, no behavior or generated output changes, other
than the testsuite outputting the value of the new flag
(always False for now).
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1468468228-27827-9-git-send-email-eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Identifier box renamed to boxed in two places]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Clean up the only remaining external use of the tag_name field of
QAPISchemaObjectTypeVariants, by explicitly listing the generated
'type' tag for all variants in the testsuite (you can still tell
simple unions by the -wrapper types). Then we can mark the
tag_name field as private by adding a leading underscore to prevent
any further use.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1468468228-27827-5-git-send-email-eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Rather than requiring all flat unions to explicitly create
a separate base struct, we can allow the qapi schema to specify
the common members via an inline dictionary. This is similar to
how commands can specify an inline anonymous type for its 'data'.
We already have several struct types that only exist to serve as
a single flat union's base; the next commit will clean them up.
In particular, this patch's change to the BlockdevOptions example
in qapi-code-gen.txt will actually be done in the real QAPI schema.
Now that anonymous bases are legal, we need to rework the
flat-union-bad-base negative test (as previously written, it
forms what is now valid QAPI; tweak it to now provide coverage
of a new error message path), and add a positive test in
qapi-schema-test to use an anonymous base (making the integer
argument optional, for even more coverage).
Note that this patch only allows anonymous bases for flat unions;
simple unions are already enough syntactic sugar that we do not
want to burden them further. Meanwhile, while it would be easy
to also allow an anonymous base for structs, that would be quite
redundant, as the members can be put right into the struct
instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1458254921-17042-15-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
The original choice of ':obj-' as the prefix for implicit types
made it obvious that we weren't going to clash with any user-defined
names, which cannot contain ':'. But now we want to create structs
for implicit types, to get rid of special cases in the generators,
and our use of ':' in implicit names needs a tweak to produce valid
C code.
We could transliterate ':' to '_', except that C99 mandates that
"identifiers that begin with an underscore are always reserved for
use as identifiers with file scope in both the ordinary and tag name
spaces". So it's time to change our naming convention: we can
instead use the 'q_' prefix that we reserved for ourselves back in
commit 9fb081e0. Technically, since we aren't planning on exposing
the empty type in generated code, we could keep the name ':empty',
but renaming it to 'q_empty' makes the check for startswith('q_')
cover all implicit types, whether or not code is generated for them.
As long as we don't declare 'empty' or 'obj' ticklish, it shouldn't
clash with c_name() prepending 'q_' to the user's ticklish names.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1458254921-17042-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
The generator special-cased
{ 'command':'foo', 'data': {} }
to avoid emitting a visitor variable, but failed to see that
{ 'struct':'NamedEmptyType, 'data': {} }
{ 'command':'foo', 'data':'NamedEmptyType' }
needs the same treatment. There, the generator happily generates a
visitor to get no arguments, and a visitor to destroy no arguments;
and the compiler isn't happy with that, as demonstrated by the updated
qapi-schema-test.json:
tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
Visitor *v;
^
No change to generated code except for the testsuite addition.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1458254921-17042-3-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Upcoming patches will adjust how we visit an object branch of an
alternate; but we were completely lacking testsuite coverage.
Rectify this, so that the future patches will be able to highlight
the changes and still prove that we avoided regressions.
In particular, the use of a flat union UserDefFlatUnion rather
than a simple struct UserDefA as the branch will give us coverage
of an object with variants. And visiting an alternate as both
the top level and as a nested member gives confidence in correct
memory allocation handling, especially if the test is run under
valgrind.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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.
Reported-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455665965-27638-1-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.
This has a couple of subtle bugs. First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS.
Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.
Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type(). Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).
However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it). A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.
This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter. This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names). Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.
Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.
There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work). Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-4-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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().
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1447836791-369-18-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1447836791-369-2-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Commit d88f5fd and friends first introduced the various test-qmp-*
tests in 2011, with duplicated hand-rolled TestStruct machinery,
to make sure the qapi visitor interface was tested. Later, commit
4f193e3 in 2013 added a .json file for further testing use by the
files, but without consolidating any of the existing hand-rolled
visitors. And with four copies, subtle differences have crept in,
between the tests themselves (mainly whitespace differences, but
also a question of whether to use NULL or "TestStruct" when
calling visit_start_struct()) and from what the generator produces
(the hand-rolled versions did not cater to partially-allocated
objects, because they did not have a deallocation usage).
Of course, just because the visitor interface is tested does not
mean it is a sane interface; and future patches will be changing
some of the visitor contracts. Rather than having to duplicate
the cleanup work in each copy of the TestStruct visitor, and keep
each hand-rolled copy in sync with what the generator supplies, we
might as well just test what the generator should give us in the
first place.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-2-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Add some testsuite coverage to ensure future patches are on
the right track:
Our current C representation of qapi arrays is done by appending
'List' to the element name; but we are not preventing the
creation of an object type with the same name. Add
reserved-type-list.json to test this. Then rename
enum-union-clash.json to reserved-type-kind.json to cover the
reservation that we DO detect, and shorten it to match the fact
that the name is reserved even if there is no clash.
We are failing to detect a collision between a dictionary member
and the implicit 'has_*' flag for another optional member. The
easiest fix would be for a future patch to reserve the entire
"has[-_]" namespace for member names (the collision is also
possible for branch names within flat unions, but only as long as
branch names can collide with (non-variant) members; however,
since future patches are about to remove that, it is not worth
testing here). Add reserved-member-has.json to test this.
A similar collision exists between a dictionary member where
c_name() munges what might otherwise be a reserved name to start
with 'q_', and another member explicitly starts with "q[-_]".
Again, the easiest solution for a future patch will be reserving
the entire namespace, but here for commands as well as members.
Add reserved-member-q.json and reserved-command-q.json to test
this; separate tests since arguably our munging of command 'unix'
to 'qmp_q_unix()' could be done without a q_, which is different
than the munging of a member 'unix' to 'foo.q_unix'.
Finally, our testsuite does not have any compilation coverage
of struct inheritance with empty qapi structs. Update
qapi-schema-test.json to test this.
Note that there is currently no technical reason to forbid type
name patterns from member names, or member name patterns from
types, since the two are not in the same namespace in C and
won't collide; but it's not worth adding positive tests of these
corner cases at this time, especially while there is other churn
pending in patches that rearrange which collisions actually
happen.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-2-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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).
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1444710158-8723-9-git-send-email-eblake@redhat.com>
[Moved array info tracking to a later patch]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
qapi-schema-test was already testing that we could have a
command returning int, but burned a command name in the whitelist.
Merge the redundant positive test returns-int, and pick a name
that reduces the whitelist size.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1444710158-8723-6-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Rather than just asserting that we can parse an empty enum,
let's also make sure we can compile it, by including it in
qapi-schema-test.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1444710158-8723-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Add some testsuite exposure for use of a 'number' as part of
an alternate. The current state of the tree has a few bugs
exposed by this: our input parser depends on the ordering of
how the qapi schema declared the alternate, and the parser
does not accept integers for a 'number' in an alternate even
though it does for numbers outside of an alternate.
Mixing 'int' and 'number' in the same alternate is unusual,
since both are supplied by json-numbers, but there does not
seem to be a technical reason to forbid it given that our
json lexer distinguishes between json-numbers that can be
represented as an int vs. those that cannot.
Improve the existing test_visitor_in_alternate() to match the
style of the new test_visitor_in_alternate_number(), and to
ensure full coverage of all possible qtype parsing.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1443565276-4535-9-git-send-email-eblake@redhat.com>
[Eric's follow-up fixes squashed in]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Expose some weaknesses in the generator: we don't always forbid
the generation of structs that contain multiple members that map
to the same C or QMP name. This has already been marked FIXME in
qapi.py in commit d90675f, but having more tests will make sure
future patches produce desired behavior; and updating existing
patches to better document things doesn't hurt, either. Some of
these collisions are already caught in the old-style parser
checks, but ultimately we want all collisions to be caught in the
new-style QAPISchema*.check() methods.
This patch focuses on C struct members, and does not consider
collisions between commands and events (affecting C function
names), or even collisions between generated C type names with
user type names (for things like automatic FOOList struct
representing array types or FOOKind for an implicit enum).
There are two types of struct collisions we want to catch:
1) Collision between two keys in a JSON object. qapi.py prevents
that within a single struct (see test duplicate-key), but it is
possible to have collisions between a type's members and its
base type's members (existing tests struct-base-clash,
struct-base-clash-deep), and its flat union variant members
(renamed test flat-union-clash-member).
2) Collision between two members of the C struct that is generated
for a given QAPI type:
a) Multiple QAPI names map to the same C name (new test
args-name-clash)
b) A QAPI name maps to a C name that is used for another purpose
(new tests flat-union-clash-branch, struct-base-clash-base,
union-clash-data). We already fixed some such cases in commit
0f61af3e and 1e6c1616, but more remain.
c) Two C names generated for other purposes clash
(updated test alternate-clash, new test union-clash-branches,
union-clash-type, flat-union-clash-type)
Ultimately, if we need to have a flat union where a tag value
clashes with a base member name, we could change the generator to
name the union (using 'foo.u.value' rather than 'foo.value') or
otherwise munge the C name corresponding to tag values. But
unless such a need arises, it will probably be easier to just
forbid these collisions.
Some of these negative tests will be deleted later, and positive
tests added to qapi-schema-test.json in their place, when the
generator code is reworked to avoid particular code generation
collisions in class 2).
[Note that viewing this patch with git rename detection enabled
may see some confusion due to renaming some tests while adding
others, but where the content is similar enough that git picks
the wrong pre- and post-patch files to associate]
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1443565276-4535-6-git-send-email-eblake@redhat.com>
[Improve commit message and comments a bit, drop an unrelated test]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
It's first class, because unlike '**', it actually works, i.e. doesn't
require 'gen': false.
'**' will go away next.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
The old code prints the result of parsing (list of expression
dictionaries), and partial results of semantic analysis (list of enum
dictionaries, list of struct dictionaries).
The new code prints a trace of a schema visit, i.e. what the back-ends
are going to use. Built-in and array types are omitted, because
they're boring.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The camel_to_upper() method applies some heuristics to turn
a mixed case type name into an all-uppercase name. This is
used for example, to generate enum constant name prefixes.
The heuristics don't also generate a satisfactory name
though. eg
{ 'enum': 'QCryptoTLSCredsEndpoint',
'data': ['client', 'server']}
Results in Q_CRYPTOTLS_CREDS_ENDPOINT_CLIENT. This has
an undesirable _ after the initial Q and is missing an
_ between the CRYPTO & TLS strings.
Rather than try to add more and more heuristics to try
to cope with this, simply allow the QAPI schema to
specify the desired enum constant prefix explicitly.
eg
{ 'enum': 'QCryptoTLSCredsEndpoint',
'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
'data': ['client', 'server']}
Now gives the QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT name.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Test case added in commit 2fc0043, and messed up in commit 5223070.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The visit_type_implicit_FOO() are generated on demand, right before
their first use. Used by visit_type_STRUCT_fields() when STRUCT has
base FOO, and by visit_type_UNION() when flat UNION has member a FOO.
If the schema defines FOO after its first use as struct base or flat
union member, visit_type_implicit_FOO() calls
visit_type_implicit_FOO() before its definition, which doesn't
compile.
Rearrange qapi-schema-test.json to demonstrate the bug.
Fix by generating the necessary forward declaration.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Enhance the testsuite to cover downstream events and commands.
Events worked without more tweaks, but commands needed a few final
updates in the generator to mangle names in the appropriate places.
In making those tweaks, it was easier to drop type_visitor() and
inline its actions instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Enhance the testsuite to cover downstream alternates, including
whether the branch name or type is downstream. Update the
generator to mangle alternate names in the appropriate places.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Enhance the testsuite to cover downstream flat unions, including
the base type, discriminator name and type, and branch name and
type. Update the generator to mangle the union names in the
appropriate places.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Enhance the testsuite to cover downstream simple unions, including
when a union branch is a downstream name. Update the generator to
mangle the union names in the appropriate places.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Enhance the testsuite to cover downstream structs, including struct
members and base structs. Update the generator to mangle the
struct names in the appropriate places.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Enhance the testsuite to cover a downstream enum type and enum
string. Update the generator to mangle the enum name in the
appropriate places.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
A future patch will be using a 'name':{dictionary} entry in the
QAPI schema to specify a default value for an optional argument;
but existing use of inline nested structs conflicts with that goal.
More precisely, a definition in the QAPI schema associates a name
with a set of properties:
Example 1: { 'struct': 'Foo', 'data': { MEMBERS... } }
associates the global name 'Foo' with properties (meta-type struct)
and MEMBERS...
Example 2: 'mumble': TYPE
within MEMBERS... above associates 'mumble' with properties (type
TYPE) and (optional false) within type Foo
The syntax of example 1 is extensible; if we need another property,
we add another name/value pair to the dictionary (such as
'base':TYPE). The syntax of example 2 is not extensible, because
the right hand side can only be a type.
We have used name encoding to add a property: "'*mumble': 'int'"
associates 'mumble' with (type int) and (optional true). Nice,
but doesn't scale. So the solution is to change our existing uses
to be syntactic sugar to an extensible form:
NAME: TYPE --> NAME: { 'type': TYPE, 'optional': false }
*ONAME: TYPE --> ONAME: { 'type': TYPE, 'optional': true }
This patch fixes the testsuite to avoid inline nested types, by
breaking the nesting into explicit types; it means that the type
is now boxed instead of unboxed in C code, but makes no difference
on the wire (and if desired, a later patch could change the
generator to not do so much boxing in C). When touching code to
add new allocations, also convert existing allocations to
consistently prefer typesafe g_new0 over g_malloc0 when a type
name is involved.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
In the testsuite, UserDefTwo and UserDefNested were identical
structs other than the member names. Reduce code duplication by
having just one type, and choose names that also favor reuse.
This will also make it easier for a later patch to get rid of
inline nested types in QAPI. When touching code related to
allocations, convert g_malloc0(sizeof(Type)) to the more typesafe
g_new0(Type, 1).
Ensure that 'make check-qapi-schema check-unit' still passes.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Referring to "type" as both a meta-type (built-in, enum, union,
alternate, or struct) and a specific type (the name that the
schema uses for declaring structs) is confusing. The confusion
is only made worse by the fact that the generator mostly already
refers to struct even when dealing with expr['type']. This
commit changes the generator to consistently refer to it as
struct everywhere, plus a single back-compat tweak that allows
accepting the existing .json files as-is, so that the meat of
this change is separate from the mindless churn of that change.
Fix the testsuite fallout for error messages that change, and
in some cases, become more legible. Improve comments to better
match our intentions where a struct (rather than any complex
type) is required. Note that in some cases, an error message
now refers to 'struct' while the schema still refers to 'type';
that will be cleaned up in the later commit to the schema.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Previous patches have led up to the point where I create the
new meta-type "'alternate':'Foo'". See the previous patches
for documentation; I intentionally split as much work into
earlier patches to minimize the size of this patch, but a lot
of it is churn due to testsuite fallout after updating to the
new type.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reduce churn in the future patch that replaces anonymous unions
with a new metatype 'alternate' by changing 'AnonUnion' to
'Alternate'.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
None of the existing QMP or QGA interfaces uses a union with a
base type but no discriminator; it is easier to avoid this in the
generator to save room for other future extensions more likely to
be useful. An earlier commit added a union-base-no-discriminator
test to ensure that we eventually give a decent error message;
likewise, removing UserDefUnion outright is okay, because we moved
all the tests we wish to keep into the tests of the simple union
UserDefNativeListUnion in the previous commit. Now is the time to
actually forbid simple union with base, and remove the last
vestiges from the testsuite.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
We were missing the 'size' builtin type (which means that QAPI using
[ 'size' ] would fail to compile).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
This is more of an exercise of the dealloc visitor, where it may
erroneously use an uninitialized discriminator field as indication
that union fields corresponding to that discriminator field/type are
present, which can lead to attempts to free random chunks of heap
memory.
Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
These cases will verify whether the expected qdict is built.
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Since enum based discriminators provide better type-safety and
ensure that future qapi additions do not forget to adjust dependent
unions, forbid using string as discriminator from now on.
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Later other scripts will need to check the enum values.
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
The test demonstrates a generator bug: the generated struct
UserDefFlatUnion doesn't include members for the indirect base
UserDefZero.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
