| Message ID | 20190910063724.28470-5-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Schema language cleanups & doc improvements | expand |
On 9/10/19 1:37 AM, Markus Armbruster wrote: > The specification claims "Each expression that isn't an include > directive may be preceded by a documentation block", but the code also > rejects them for pragma directives. The code is correct. Fix the > specification. > > The specification reserves member names starting with 'has_', but the > code also reserves name 'u'. Fix the specification. Reservation of 'u' was done in 5e59baf9 (and claimed we could add a munge to q_u in the future if we ever needed a name 'u' after all). > > The specification claims "The string 'max' is not allowed as an enum > value". Untrue. Fix the specification. While there, delete the > naming advice, because it's redundant with the naming rules in section > "Schema overview" Used to be true; missed when commit 7fb1cf16 got rid of the collision. > > The specification claims "No branch of the union can be named 'max', > as this would collide with the implicit enum". Untrue. Fix the > specification. Fixed around the same time (although I didn't check if it was in the same commit) > > The specification claims "It is not allowed to name an event 'MAX', > since the generator also produces a C enumeration of all event names > with a generated _MAX value at the end." Untrue. Fix the > specification. And similar comment. I don't know if you want to do exact commit ids where all of these doc problems were introduced (because of code patches that lifted the limitations). > > The specification claims "All branches of the union must be complex > types", but the code permits only struct types. The code is correct. > Fix the specification. > > The specification claims a command's return type "must be the string > name of a complex or built-in type, a one-element array containing the > name of a complex or built-in type" unless the command is in pragma > 'returns-whitelist'. The code does not permit built-in types. Fix > the specification. Umm: qapi/migration.json:{ 'command': 'query-migrate-cache-size', 'returns': 'int' } I don't know if we use an array of a built-in-type, but we definitely have unfortunate commands that return a non-JSON-object. [1] > A flat union definition avoids nesting on the wire, and specifies a > set of common members that occur in all variants of the union. The > 'base' key must specify either a type name (the type must be a > struct, not a union), or a dictionary representing an anonymous type. > -All branches of the union must be complex types, and the top-level > +All branches of the union must be struct types, and the top-level We have hit cases where it might have been nicer to permit a flat union whose branch is itself another flat union. But until we actually code that up to work, this is accurate. > @@ -578,8 +578,8 @@ The 'returns' member describes what will appear in the "return" member > of a Client JSON Protocol reply on successful completion of a command. > The member is optional from the command declaration; if absent, the > "return" member will be an empty dictionary. If 'returns' is present, > -it must be the string name of a complex or built-in type, a > -one-element array containing the name of a complex or built-in type. > +it must be the string name of a complex type, or a > +one-element array containing the name of a complex type. > To return anything else, you have to list the command in pragma > 'returns-whitelist'. If you do this, the command cannot be extended > to return additional information in the future. Use of [1] Aha - it's 'returns-whitelist' that makes the difference. Okay, your wording change here makes sense: a built-in is NOT permitted UNLESS you whitelist it. Summary: you may want to improve the commit message with git archaeology, but the wording changes themselves make sense. Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 9/10/19 1:37 AM, Markus Armbruster wrote: >> The specification claims "Each expression that isn't an include >> directive may be preceded by a documentation block", but the code also >> rejects them for pragma directives. The code is correct. Fix the >> specification. >> >> The specification reserves member names starting with 'has_', but the >> code also reserves name 'u'. Fix the specification. > > Reservation of 'u' was done in 5e59baf9 (and claimed we could add a > munge to q_u in the future if we ever needed a name 'u' after all). Yes. has_ could be munged away, too. >> The specification claims "The string 'max' is not allowed as an enum >> value". Untrue. Fix the specification. While there, delete the >> naming advice, because it's redundant with the naming rules in section >> "Schema overview" > > Used to be true; missed when commit 7fb1cf16 got rid of the collision. Correct. >> The specification claims "No branch of the union can be named 'max', >> as this would collide with the implicit enum". Untrue. Fix the >> specification. > > Fixed around the same time (although I didn't check if it was in the > same commit) > >> >> The specification claims "It is not allowed to name an event 'MAX', >> since the generator also produces a C enumeration of all event names >> with a generated _MAX value at the end." Untrue. Fix the >> specification. > > And similar comment. > > I don't know if you want to do exact commit ids where all of these doc > problems were introduced (because of code patches that lifted the > limitations). I'm (overly?) fond of git archeology myself, but I found these bugs while fighting crocodiles in the swamp, so I couldn't indulge. >> The specification claims "All branches of the union must be complex >> types", but the code permits only struct types. The code is correct. >> Fix the specification. >> >> The specification claims a command's return type "must be the string >> name of a complex or built-in type, a one-element array containing the >> name of a complex or built-in type" unless the command is in pragma >> 'returns-whitelist'. The code does not permit built-in types. Fix >> the specification. > > Umm: > > qapi/migration.json:{ 'command': 'query-migrate-cache-size', 'returns': > 'int' } > > I don't know if we use an array of a built-in-type, but we definitely > have unfortunate commands that return a non-JSON-object. [1] > >> A flat union definition avoids nesting on the wire, and specifies a >> set of common members that occur in all variants of the union. The >> 'base' key must specify either a type name (the type must be a >> struct, not a union), or a dictionary representing an anonymous type. >> -All branches of the union must be complex types, and the top-level >> +All branches of the union must be struct types, and the top-level > > We have hit cases where it might have been nicer to permit a flat union > whose branch is itself another flat union. But until we actually code > that up to work, this is accurate. > > >> @@ -578,8 +578,8 @@ The 'returns' member describes what will appear in the "return" member >> of a Client JSON Protocol reply on successful completion of a command. >> The member is optional from the command declaration; if absent, the >> "return" member will be an empty dictionary. If 'returns' is present, >> -it must be the string name of a complex or built-in type, a >> -one-element array containing the name of a complex or built-in type. >> +it must be the string name of a complex type, or a >> +one-element array containing the name of a complex type. >> To return anything else, you have to list the command in pragma >> 'returns-whitelist'. If you do this, the command cannot be extended >> to return additional information in the future. Use of > > [1] Aha - it's 'returns-whitelist' that makes the difference. Okay, > your wording change here makes sense: a built-in is NOT permitted UNLESS > you whitelist it. > > Summary: you may want to improve the commit message with git > archaeology, but the wording changes themselves make sense. I'll see what I can do without too much effort. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 3d3931fb7a..4ce67752a7 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -117,9 +117,9 @@ Example: ==== Expression documentation ==== -Each expression that isn't an include directive may be preceded by a -documentation block. Such blocks are called expression documentation -blocks. +Expressions other than include and pragma directives may be preceded +by a documentation block. Such blocks are called expression +documentation blocks. When documentation is required (see pragma 'doc-required'), expression documentation blocks are mandatory. @@ -243,8 +243,9 @@ underscore. Event names should be ALL_CAPS with words separated by underscore. -Member names starting with 'has-' or 'has_' are reserved for the -generator, which uses them for tracking optional members. +Member name 'u' and names starting with 'has-' or 'has_' are reserved +for the generator, which uses them for unions and for tracking +optional members. Any name (command, event, type, member, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed @@ -460,15 +461,14 @@ discriminator value, as in these examples: The generated C code uses a struct containing a union. Additionally, an implicit C enum 'NameKind' is created, corresponding to the union -'Name', for accessing the various branches of the union. No branch of -the union can be named 'max', as this would collide with the implicit -enum. The value for each branch can be of any type. +'Name', for accessing the various branches of the union. The value +for each branch can be of any type. A flat union definition avoids nesting on the wire, and specifies a set of common members that occur in all variants of the union. The 'base' key must specify either a type name (the type must be a struct, not a union), or a dictionary representing an anonymous type. -All branches of the union must be complex types, and the top-level +All branches of the union must be struct types, and the top-level members of the union dictionary on the wire will be combination of members from both the base type and the appropriate branch type (when merging two dictionaries, there must be no keys in common). The @@ -578,8 +578,8 @@ The 'returns' member describes what will appear in the "return" member of a Client JSON Protocol reply on successful completion of a command. The member is optional from the command declaration; if absent, the "return" member will be an empty dictionary. If 'returns' is present, -it must be the string name of a complex or built-in type, a -one-element array containing the name of a complex or built-in type. +it must be the string name of a complex type, or a +one-element array containing the name of a complex type. To return anything else, you have to list the command in pragma 'returns-whitelist'. If you do this, the command cannot be extended to return additional information in the future. Use of @@ -691,13 +691,11 @@ started with --preconfig. Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, '*boxed': true } -Events are defined with the keyword 'event'. It is not allowed to -name an event 'MAX', since the generator also produces a C enumeration -of all event names with a generated _MAX value at the end. When -'data' is also specified, additional info will be included in the -event, with similar semantics to a 'struct' expression. Finally there -will be C API generated in qapi-events.h; when called by QEMU code, a -message with timestamp will be emitted on the wire. +Events are defined with the keyword 'event'. When 'data' is also +specified, additional info will be included in the event, with similar +semantics to a 'struct' expression. Finally there will be C API +generated in qapi-events.h; when called by QEMU code, a message with +timestamp will be emitted on the wire. An example event is:
The specification claims "Each expression that isn't an include directive may be preceded by a documentation block", but the code also rejects them for pragma directives. The code is correct. Fix the specification. The specification reserves member names starting with 'has_', but the code also reserves name 'u'. Fix the specification. The specification claims "The string 'max' is not allowed as an enum value". Untrue. Fix the specification. While there, delete the naming advice, because it's redundant with the naming rules in section "Schema overview" The specification claims "No branch of the union can be named 'max', as this would collide with the implicit enum". Untrue. Fix the specification. The specification claims "It is not allowed to name an event 'MAX', since the generator also produces a C enumeration of all event names with a generated _MAX value at the end." Untrue. Fix the specification. The specification claims "All branches of the union must be complex types", but the code permits only struct types. The code is correct. Fix the specification. The specification claims a command's return type "must be the string name of a complex or built-in type, a one-element array containing the name of a complex or built-in type" unless the command is in pragma 'returns-whitelist'. The code does not permit built-in types. Fix the specification. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/devel/qapi-code-gen.txt | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)