Re: [Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Eric Blake <eblake@redhat.com>]

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

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> Reproducer: with
> 
>     { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }
> 
> added to qapi-schema-test.json, qapi-commands.py dies when it tries to
> generate the command handler function
> 
>     Traceback (most recent call last):
>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 359, in <module>
>         ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 29, in generate_command_decl
>         ret_type=c_type(ret_type), name=c_name(name),
>       File "/work/armbru/qemu/scripts/qapi.py", line 927, in c_type
>         assert isinstance(value, str) and value != ""
>     AssertionError
> 
> because the return type doesn't exist.
> 
> Simply outlaw this usage.

Might be worth allowing someday, but that would imply that we can come
up with a sane naming scheme for anonymous structs in the qapi schema
that won't risk collisions with explicit types.  Shame on me for not
thinking to test this in my earlier testsuite additions, but I
definitely agree with your solution of outlawing it for now.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt                                  | 17 ++++++++---------
>  scripts/qapi.py                                         |  2 +-
>  tests/Makefile                                          |  4 ++--
>  tests/qapi-schema/nested-struct-returns.err             |  1 -
>  tests/qapi-schema/nested-struct-returns.json            |  3 ---
>  tests/qapi-schema/returns-dict.err                      |  1 +
>  .../{nested-struct-returns.exit => returns-dict.exit}   |  0
>  tests/qapi-schema/returns-dict.json                     |  2 ++
>  .../{nested-struct-returns.out => returns-dict.out}     |  0
>  9 files changed, 14 insertions(+), 16 deletions(-)

Once again, git rename detection didn't accurately capture what you did :)

>  delete mode 100644 tests/qapi-schema/nested-struct-returns.err
>  delete mode 100644 tests/qapi-schema/nested-struct-returns.json
>  create mode 100644 tests/qapi-schema/returns-dict.err
>  rename tests/qapi-schema/{nested-struct-returns.exit => returns-dict.exit} (100%)
>  create mode 100644 tests/qapi-schema/returns-dict.json
>  rename tests/qapi-schema/{nested-struct-returns.out => returns-dict.out} (100%)
> 

git grep "'returns'.*{"

found a couple more culprits (tests that fail elsewhere prior to warning
about this, but where an anonymous return does not add to the negative
test).  Please squash this in:

diff --git i/tests/qapi-schema/command-int.json
w/tests/qapi-schema/command-int.json
index c90d408..40a6ae3 100644
--- i/tests/qapi-schema/command-int.json
+++ w/tests/qapi-schema/command-int.json
@@ -1,3 +1,4 @@
 # we reject collisions between commands and types
 { 'command': 'int', 'data': { 'character': 'str' },
-  'returns': { 'value': 'int' } }
+  'returns': 'Foo' }
+{ 'struct': 'Foo', 'data': { 'value': 'int' } }
diff --git i/tests/qapi-schema/nested-struct-data.json
w/tests/qapi-schema/nested-struct-data.json
index 3d52d2b..efbe773 100644
--- i/tests/qapi-schema/nested-struct-data.json
+++ w/tests/qapi-schema/nested-struct-data.json
@@ -1,4 +1,3 @@
 # inline subtypes collide with our desired future use of defaults
 { 'command': 'foo',
-  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' },
-  'returns': {} }
+  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }


at which point you may add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]