Re: [Qemu-devel] [PATCH v2 04/16] docs/devel/qapi-code-gen: Minor specification fixes

[Markus Armbruster <armbru@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!