| Message ID | 55C0FE8A.9040000@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <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' } } 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 <eblake@redhat.com> Thanks!
On 08/04/2015 11:29 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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 >>> >> +++ 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? > Sure, that works too.
Eric Blake <eblake@redhat.com> writes: > On 08/04/2015 11:29 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 >>>> > >>> +++ 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? >> > > Sure, that works too. Done, with commit message amended: Simply outlaw this usage, and drop or dumb down test cases accordingly.
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' } }