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

[Eric Blake]

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 <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature