Re: [Qemu-devel] [PATCH RFC v2 44/47] qapi: Pseudo-type '**' is now unused, drop it

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3902 bytes --]

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> 'gen': false needs to stay for now, because netdev_add is still using
> it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt                               | 15 ++++-----------
>  scripts/qapi.py                                      | 20 ++++----------------
>  tests/Makefile                                       |  4 ++--
>  tests/qapi-schema/args-returns-any.err               |  1 +
>  ...type-bypass-no-gen.exit => args-returns-any.exit} |  0

> 
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2367c66..ca578dd 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt

> @@ -457,13 +454,9 @@ which would validate this Client JSON Protocol transaction:
>  
>  In rare cases, QAPI cannot express a type-safe representation of a
>  corresponding Client JSON Protocol command.  In these cases, if the
> -command expression includes the key 'gen' with boolean value false,
> -then the 'data' or 'returns' member that intends to bypass generated
> -type-safety and do its own manual validation should use an inline
> -dictionary definition, with a value of '**' rather than a valid type
> -name for the keys that the generated code will not validate.  Please
> -try to avoid adding new commands that rely on this, and instead use
> -type-safe unions.  For an example of bypass usage:
> +command expression includes the key 'gen' with boolean value false.

Incomplete sentence.  "if the command expression includes... false"
needs "then something...".

> +Please try to avoid adding new commands that rely on this, and instead
> +use type-safe unions.  For an example of bypass usage:

And given my ideas, we may be able to make netdev_add typesafe and
eliminate 'gen' altogether in the future :)

> +++ b/tests/Makefile
> @@ -222,11 +222,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>  	bad-type-dict.json double-data.json unknown-expr-key.json \
>  	redefined-type.json redefined-command.json redefined-builtin.json \
>  	redefined-event.json command-int.json bad-data.json event-max.json \
> -	type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
> +	type-bypass-bad-gen.json \

If I understand correctly (git didn't quite guess right),
type-bypass-no-gen.json disappears completely (we no longer have working
'**'), and type-bypass.json...

>  	data-array-empty.json data-array-unknown.json data-int.json \
>  	data-unknown.json data-member-unknown.json data-member-array.json \
>  	data-member-array-bad.json args-union.json args-alternate.json \
> -	returns-array-bad.json returns-int.json \
> +	args-returns-any.json returns-array-bad.json returns-int.json \

...gets renamed args-returns-any.json (because type bypass is now
expressed via 'any'.

> +# built-in type 'any' in arguments and returns
> +# works except for 'data': 'any', because that has to be a struct
> +# note: command name 'qom-get' chosen to avoid "cannot use built-in" error
> +{ 'command': 'qom-get', 'data': { 'arg': 'any' }, 'returns': 'any' }
> +{ 'command': 'foo', 'returns': { 'arg': 'any' } }
> +{ 'command': 'bar', 'data': 'any' }

This shows what the parser accepts and rejects, but doesn't prove it
compiles to C; and the previous patch that touched
test-qmp-input-visitor.c was testing that the low-level interactions
worked but did not map it all the way back to qapi.  I wonder if you
should also enhance qapi-schema-test to use 'any', since that tends to
be the one test that most fully exercises interactions with generated
code based on a valid qapi schema.


On the right track, but I want to make sure the botched documentation is
correct before giving R-b.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]