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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Okay to simply drop the 'returns' instead?

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

Thanks!