Message ID | 1463784024-17242-8-git-send-email-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > Turn on the ability to pass command and event arguments in > a single boxed parameter. For structs, it makes it possible > to pass a single qapi type instead of a breakout of all > struct members; for unions, it is now possible to use a > union as the data for a command or event. > > Generated code is unchanged, as long as no client uses the > new feature. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v7: rebase to latest > v6: retitle, rebase, and merge v5 40/46 and 41/46 into one patch > --- > scripts/qapi.py | 29 +++++++++++++++++++++-------- > scripts/qapi-commands.py | 3 ++- > scripts/qapi-event.py | 5 ++++- > tests/test-qmp-commands.c | 12 ++++++++++++ > docs/qapi-code-gen.txt | 24 ++++++++++++++++++++++-- > tests/Makefile | 2 ++ > tests/qapi-schema/args-bad-box.err | 1 + > tests/qapi-schema/args-bad-box.exit | 1 + > tests/qapi-schema/args-bad-box.json | 2 ++ > tests/qapi-schema/args-bad-box.out | 0 > tests/qapi-schema/args-box-anon.err | 1 + > tests/qapi-schema/args-box-anon.exit | 1 + > tests/qapi-schema/args-box-anon.json | 2 ++ > tests/qapi-schema/args-box-anon.out | 0 > tests/qapi-schema/args-union.err | 2 +- > tests/qapi-schema/args-union.json | 3 +-- > tests/qapi-schema/qapi-schema-test.json | 6 +++++- > tests/qapi-schema/qapi-schema-test.out | 10 +++++++++- > 18 files changed, 87 insertions(+), 17 deletions(-) > create mode 100644 tests/qapi-schema/args-bad-box.err > create mode 100644 tests/qapi-schema/args-bad-box.exit > create mode 100644 tests/qapi-schema/args-bad-box.json > create mode 100644 tests/qapi-schema/args-bad-box.out > create mode 100644 tests/qapi-schema/args-box-anon.err > create mode 100644 tests/qapi-schema/args-box-anon.exit > create mode 100644 tests/qapi-schema/args-box-anon.json > create mode 100644 tests/qapi-schema/args-box-anon.out > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 108363d..6742e7a 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -522,10 +522,14 @@ def check_type(expr_info, source, value, allow_array=False, > > def check_command(expr, expr_info): > name = expr['command'] > + box = expr.get('box', False) > > + args_meta = ['struct'] > + if box: > + args_meta += ['union'] > check_type(expr_info, "'data' for command '%s'" % name, > - expr.get('data'), allow_dict=True, allow_optional=True, > - allow_metas=['struct']) > + expr.get('data'), allow_dict=not box, allow_optional=True, > + allow_metas=args_meta) > returns_meta = ['union', 'struct'] > if name in returns_whitelist: > returns_meta += ['built-in', 'alternate', 'enum'] > @@ -537,11 +541,15 @@ def check_command(expr, expr_info): > def check_event(expr, expr_info): > global events > name = expr['event'] > + box = expr.get('box', False) > + meta = ['struct'] > > + if box: > + meta += ['union'] > events.append(name) > check_type(expr_info, "'data' for event '%s'" % name, > - expr.get('data'), allow_dict=True, allow_optional=True, > - allow_metas=['struct']) > + expr.get('data'), allow_dict=not box, allow_optional=True, > + allow_metas=meta) > > > def check_union(expr, expr_info): > @@ -694,6 +702,10 @@ def check_keys(expr_elem, meta, required, optional=[]): > raise QAPIExprError(info, > "'%s' of %s '%s' should only use false value" > % (key, meta, name)) > + if key == 'box' and value is not True: > + raise QAPIExprError(info, > + "'%s' of %s '%s' should only use true value" > + % (key, meta, name)) > for key in required: > if key not in expr: > raise QAPIExprError(info, > @@ -725,10 +737,10 @@ def check_exprs(exprs): > add_struct(expr, info) > elif 'command' in expr: > check_keys(expr_elem, 'command', [], > - ['data', 'returns', 'gen', 'success-response']) > + ['data', 'returns', 'gen', 'success-response', 'box']) > add_name(expr['command'], info, 'command') > elif 'event' in expr: > - check_keys(expr_elem, 'event', [], ['data']) > + check_keys(expr_elem, 'event', [], ['data', 'box']) > add_name(expr['event'], info, 'event') > else: > raise QAPIExprError(expr_elem['info'], > @@ -1640,12 +1652,13 @@ extern const char *const %(c_name)s_lookup[]; > > > def gen_params(arg_type, box, extra): > - if not arg_type: > + if not arg_type or arg_type.is_empty(): > return extra When arg_type is empty, box gets ignored. That's not wrong, but I'd skin this cat differently: when box=true, pass a single arg_type argument, no matter what the type is. > ret = '' > sep = '' > if box: > - assert False # not implemented > + ret += '%s arg' % arg_type.c_param_type() > + sep = ', ' > else: > for memb in arg_type.members: > ret += sep > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 79d4eea..dc41fad 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -30,7 +30,8 @@ def gen_call(name, arg_type, box, ret_type): > > argstr = '' > if box: > - assert False # not implemented > + if arg_type and not arg_type.is_empty(): > + argstr = '&arg, ' Related. > elif arg_type: > assert not arg_type.variants > for memb in arg_type.members: > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index fd953fe..b8ca8c8 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -96,7 +96,10 @@ def gen_event_send(name, arg_type, box): > ''') > > if box: > - assert False # not implemented > + ret += mcgen(''' > + visit_type_%(c_name)s(v, NULL, &arg, &err); > +''', > + c_name=arg_type.c_name(), name=arg_type.name) > else: > ret += mcgen(''' > visit_start_struct(v, "%(name)s", NULL, 0, &err); Same design alternative for empty types, only here it's not visible in the patch, due to your split of the implementation between here and the previous patch. > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > index 87fc759..007db37 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -60,6 +60,18 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp) > return arg; > } > > +void qmp_boxed_empty(Error **errp) > +{ > +} Demontrates that 'box': true with an empty type isn't boxed. > + > +void qmp_boxed_struct(UserDefZero *arg, Error **errp) > +{ > +} > + > +void qmp_boxed_union(UserDefNativeListUnion *arg, Error **errp) > +{ > +} > + > __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a, > __org_qemu_x_StructList *b, > __org_qemu_x_Union2 *c, > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index aefc29e..40f050e 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -410,7 +410,7 @@ following example objects: > === Commands === > > Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, > - '*returns': TYPE-NAME, > + '*returns': TYPE-NAME, '*box': true, > '*gen': false, '*success-response': false } > > Commands are defined by using a dictionary containing several members, > @@ -461,6 +461,17 @@ which would validate this Client JSON Protocol transaction: > => { "execute": "my-second-command" } > <= { "return": [ { "value": "one" }, { } ] } > > +By default, the generator creates a marshalling function that converts > +an input QDict into a function call implemented by the user, and Well, the called function is implemented by the user. > +declares a prototype for the user's function which has a parameter for > +each member of the argument struct, including boolean arguments that > +describe whether optional arguments were provided. But if the QAPI > +description includes the key 'box' with the boolean value true, the > +user call prototype will have only a single parameter for the overall > +generated C structure. The 'box' key is required in order to use a > +union as an input argument, since it is not possible to list all > +members of the union as separate parameters. > + Neglects to mention that 'data' is less restricted with 'box': true. Suggest: The generator emits a prototype for the user's function implementing the command. Normally, 'data' is or names a struct type, and its members are passed as separate arguments to this function. If the command definition includes a key 'box' with the boolean value true, then the arguments are passed to the function as a single pointer to the QAPI type generated for 'data'. 'data' may name an arbitrary complex type then. This still glosses over the has_ arguments, but it's no worse than before. Only then mention the marshalling: The generator emits a marshalling function that extracts arguments for the user's function out of an input QDict, calls the user's function, and if it succeeded, builds an output QObject from its return value. > In rare cases, QAPI cannot express a type-safe representation of a > corresponding Client JSON Protocol command. You then have to suppress > generation of a marshalling function by including a key 'gen' with > @@ -484,7 +495,8 @@ use of this member. > > === Events === > > -Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT } > +Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, > + '*box': true } > > Events are defined with the keyword 'event'. It is not allowed to > name an event 'MAX', since the generator also produces a C enumeration > @@ -505,6 +517,14 @@ Resulting in this JSON object: > "data": { "b": "test string" }, > "timestamp": { "seconds": 1267020223, "microseconds": 435656 } } > > +By default, the generator creates a C function that takes as > +parameters each member of the argument struct and turns it into the > +appropriate JSON Client event. But if the QAPI description includes > +the key 'box' with the boolean value true, the event function will > +have only a single parameter for the overall generated C structure. > +The 'box' key is required in order to use a union as the data key, > +since it is not possible to list all members of the union as separate > +parameters. Suggest: The generator emits a function to send the event. Normally, 'data' is or names a struct type, and its members are passed as separate arguments to this function. If the event definition includes a key 'box' with the boolean value true, then the arguments are passed to the function as a single pointer to the QAPI type generated for 'data'. 'data' may name an arbitrary complex type then. > > == Client JSON Protocol introspection == > > diff --git a/tests/Makefile b/tests/Makefile > index c608f9a..5cd6177 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -272,6 +272,8 @@ qapi-schema += args-alternate.json > qapi-schema += args-any.json > qapi-schema += args-array-empty.json > qapi-schema += args-array-unknown.json > +qapi-schema += args-bad-box.json > +qapi-schema += args-box-anon.json > qapi-schema += args-int.json > qapi-schema += args-invalid.json > qapi-schema += args-member-array-bad.json > diff --git a/tests/qapi-schema/args-bad-box.err b/tests/qapi-schema/args-bad-box.err > new file mode 100644 > index 0000000..16afe3c > --- /dev/null > +++ b/tests/qapi-schema/args-bad-box.err > @@ -0,0 +1 @@ > +tests/qapi-schema/args-bad-box.json:2: 'box' of command 'foo' should only use true value > diff --git a/tests/qapi-schema/args-bad-box.exit b/tests/qapi-schema/args-bad-box.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/args-bad-box.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/args-bad-box.json b/tests/qapi-schema/args-bad-box.json > new file mode 100644 > index 0000000..8d5737a > --- /dev/null > +++ b/tests/qapi-schema/args-bad-box.json > @@ -0,0 +1,2 @@ > +# 'box' should only appear with value true > +{ 'command': 'foo', 'box': false } > diff --git a/tests/qapi-schema/args-bad-box.out b/tests/qapi-schema/args-bad-box.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/args-box-anon.err b/tests/qapi-schema/args-box-anon.err > new file mode 100644 > index 0000000..11eaefc > --- /dev/null > +++ b/tests/qapi-schema/args-box-anon.err > @@ -0,0 +1 @@ > +tests/qapi-schema/args-box-anon.json:2: 'data' for command 'foo' should be a type name > diff --git a/tests/qapi-schema/args-box-anon.exit b/tests/qapi-schema/args-box-anon.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/args-box-anon.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/args-box-anon.json b/tests/qapi-schema/args-box-anon.json > new file mode 100644 > index 0000000..947e3c6 > --- /dev/null > +++ b/tests/qapi-schema/args-box-anon.json > @@ -0,0 +1,2 @@ > +# 'box' can only be used with named types > +{ 'command': 'foo', 'box': true, 'data': { 'string': 'str' } } > diff --git a/tests/qapi-schema/args-box-anon.out b/tests/qapi-schema/args-box-anon.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err > index 1d693d7..f8ad223 100644 > --- a/tests/qapi-schema/args-union.err > +++ b/tests/qapi-schema/args-union.err > @@ -1 +1 @@ > -tests/qapi-schema/args-union.json:4: 'data' for command 'oops' cannot use union type 'Uni' > +tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use union type 'Uni' > diff --git a/tests/qapi-schema/args-union.json b/tests/qapi-schema/args-union.json > index 7bdcbb7..c0ce091 100644 > --- a/tests/qapi-schema/args-union.json > +++ b/tests/qapi-schema/args-union.json > @@ -1,4 +1,3 @@ > -# we do not allow union arguments > -# TODO should we support this? > +# use of union arguments requires 'box':true > { 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } } > { 'command': 'oops', 'data': 'Uni' } > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index f571e1b..df91f3d 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -127,6 +127,9 @@ > { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' }, > 'returns': 'int' } > { 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' } > +{ 'command': 'boxed-empty', 'box': true } > +{ 'command': 'boxed-struct', 'box': true, 'data': 'UserDefZero' } > +{ 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'box': true } > > # For testing integer range flattening in opts-visitor. The following schema > # corresponds to the option format: > @@ -147,13 +150,14 @@ > { 'struct': 'EventStructOne', > 'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } } > > -{ 'event': 'EVENT_A' } > +{ 'event': 'EVENT_A', 'box': true } This is case "empty". Separate tests for both values of box would be cleaner, even though they produce the exact same result. If we decide to obey box even with empty types, they don't. > { 'event': 'EVENT_B', > 'data': { } } > { 'event': 'EVENT_C', > 'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } } > { 'event': 'EVENT_D', > 'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } } > +{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' } This is case "struct". Missing: case "union". > > # test that we correctly compile downstream extensions, as well as munge > # ticklish names > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 754a926..8a00c6b 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -23,13 +23,15 @@ alternate AltStrNum > case s: str > case n: number > event EVENT_A None > - box=False > + box=True > event EVENT_B None > box=False > event EVENT_C q_obj_EVENT_C-arg > box=False > event EVENT_D q_obj_EVENT_D-arg > box=False > +event EVENT_E UserDefZero > + box=True > object Empty1 > object Empty2 > base Empty1 > @@ -153,6 +155,12 @@ object __org.qemu_x-Union2 > case __org.qemu_x-value: __org.qemu_x-Struct2 > command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1 > gen=True success_response=True box=False > +command boxed-empty None -> None > + gen=True success_response=True box=True > +command boxed-struct UserDefZero -> None > + gen=True success_response=True box=True > +command boxed-union UserDefNativeListUnion -> None > + gen=True success_response=True box=True > command guest-get-time q_obj_guest-get-time-arg -> int > gen=True success_response=True box=False > command guest-sync q_obj_guest-sync-arg -> any
On 06/14/2016 09:27 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Turn on the ability to pass command and event arguments in >> a single boxed parameter. For structs, it makes it possible >> to pass a single qapi type instead of a breakout of all >> struct members; for unions, it is now possible to use a >> union as the data for a command or event. >> >> Generated code is unchanged, as long as no client uses the >> new feature. >> >> @@ -1640,12 +1652,13 @@ extern const char *const %(c_name)s_lookup[]; >> >> >> def gen_params(arg_type, box, extra): >> - if not arg_type: >> + if not arg_type or arg_type.is_empty(): >> return extra > > When arg_type is empty, box gets ignored. That's not wrong, but I'd > skin this cat differently: when box=true, pass a single arg_type > argument, no matter what the type is. Except that we don't have a visit function generated for the 'q_empty' type; I'm worried that coming up with the right arg_type for an empty box may be difficult. >> +++ b/tests/test-qmp-commands.c >> @@ -60,6 +60,18 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp) >> return arg; >> } >> >> +void qmp_boxed_empty(Error **errp) >> +{ >> +} > > Demontrates that 'box': true with an empty type isn't boxed. In other words, an empty type takes precedence over 'box':true, because there is nothing to be passed. I could go the other direction and make it a hard error to use 'box':true on an empty type, if that would be conceptually cleaner. >> >> +By default, the generator creates a marshalling function that converts >> +an input QDict into a function call implemented by the user, and > > Well, the called function is implemented by the user. > >> +declares a prototype for the user's function which has a parameter for >> +each member of the argument struct, including boolean arguments that >> +describe whether optional arguments were provided. But if the QAPI >> +description includes the key 'box' with the boolean value true, the >> +user call prototype will have only a single parameter for the overall >> +generated C structure. The 'box' key is required in order to use a >> +union as an input argument, since it is not possible to list all >> +members of the union as separate parameters. >> + > > Neglects to mention that 'data' is less restricted with 'box': true. > > Suggest: > > The generator emits a prototype for the user's function implementing > the command. Normally, 'data' is or names a struct type, and its > members are passed as separate arguments to this function. If the > command definition includes a key 'box' with the boolean value true, > then the arguments are passed to the function as a single pointer to > the QAPI type generated for 'data'. 'data' may name an arbitrary > complex type then. Or maybe arbitrary non-empty complex type, depending on what we decide above. And maybe I still need to make it clear that when using 'box':true, an anonymous type is no longer permitted. > > This still glosses over the has_ arguments, but it's no worse than > before. > > Only then mention the marshalling: > > The generator emits a marshalling function that extracts arguments > for the user's function out of an input QDict, calls the user's > function, and if it succeeded, builds an output QObject from its > return value. Sure, I can reword along those lines. (I may not state it enough, but thanks for your wordsmithing help). >> @@ -147,13 +150,14 @@ >> { 'struct': 'EventStructOne', >> 'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } } >> >> -{ 'event': 'EVENT_A' } >> +{ 'event': 'EVENT_A', 'box': true } > > This is case "empty". > > Separate tests for both values of box would be cleaner, even though they > produce the exact same result. If we decide to obey box even with empty > types, they don't. > Or, if we decide to forbid 'box':true on an empty type, then this needs tweaking anyway. >> { 'event': 'EVENT_B', >> 'data': { } } >> { 'event': 'EVENT_C', >> 'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } } >> { 'event': 'EVENT_D', >> 'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } } >> +{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' } > > This is case "struct". > > Missing: case "union". >
Eric Blake <eblake@redhat.com> writes: > On 06/14/2016 09:27 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Turn on the ability to pass command and event arguments in >>> a single boxed parameter. For structs, it makes it possible >>> to pass a single qapi type instead of a breakout of all >>> struct members; for unions, it is now possible to use a >>> union as the data for a command or event. >>> >>> Generated code is unchanged, as long as no client uses the >>> new feature. >>> > >>> @@ -1640,12 +1652,13 @@ extern const char *const %(c_name)s_lookup[]; >>> >>> >>> def gen_params(arg_type, box, extra): >>> - if not arg_type: >>> + if not arg_type or arg_type.is_empty(): >>> return extra >> >> When arg_type is empty, box gets ignored. That's not wrong, but I'd >> skin this cat differently: when box=true, pass a single arg_type >> argument, no matter what the type is. > > Except that we don't have a visit function generated for the 'q_empty' > type; I'm worried that coming up with the right arg_type for an empty > box may be difficult. > > >>> +++ b/tests/test-qmp-commands.c >>> @@ -60,6 +60,18 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp) >>> return arg; >>> } >>> >>> +void qmp_boxed_empty(Error **errp) >>> +{ >>> +} >> >> Demontrates that 'box': true with an empty type isn't boxed. > > In other words, an empty type takes precedence over 'box':true, because > there is nothing to be passed. > > I could go the other direction and make it a hard error to use > 'box':true on an empty type, if that would be conceptually cleaner. That's fine with me. We can always relax the restriction if it turns out bothersome. >>> +By default, the generator creates a marshalling function that converts >>> +an input QDict into a function call implemented by the user, and >> >> Well, the called function is implemented by the user. >> >>> +declares a prototype for the user's function which has a parameter for >>> +each member of the argument struct, including boolean arguments that >>> +describe whether optional arguments were provided. But if the QAPI >>> +description includes the key 'box' with the boolean value true, the >>> +user call prototype will have only a single parameter for the overall >>> +generated C structure. The 'box' key is required in order to use a >>> +union as an input argument, since it is not possible to list all >>> +members of the union as separate parameters. >>> + >> >> Neglects to mention that 'data' is less restricted with 'box': true. >> >> Suggest: >> >> The generator emits a prototype for the user's function implementing >> the command. Normally, 'data' is or names a struct type, and its >> members are passed as separate arguments to this function. If the >> command definition includes a key 'box' with the boolean value true, >> then the arguments are passed to the function as a single pointer to >> the QAPI type generated for 'data'. 'data' may name an arbitrary >> complex type then. > > Or maybe arbitrary non-empty complex type, depending on what we decide > above. And maybe I still need to make it clear that when using > 'box':true, an anonymous type is no longer permitted. My text kind of implies "anonymous not permitted": "'data' may *name* an arbitrary complex type then" (emphasis added). Stating it explictly would be better. >> This still glosses over the has_ arguments, but it's no worse than >> before. >> >> Only then mention the marshalling: >> >> The generator emits a marshalling function that extracts arguments >> for the user's function out of an input QDict, calls the user's >> function, and if it succeeded, builds an output QObject from its >> return value. > > Sure, I can reword along those lines. (I may not state it enough, but > thanks for your wordsmithing help). Thanks for caring for it! >>> @@ -147,13 +150,14 @@ >>> { 'struct': 'EventStructOne', >>> 'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } } >>> >>> -{ 'event': 'EVENT_A' } >>> +{ 'event': 'EVENT_A', 'box': true } >> >> This is case "empty". >> >> Separate tests for both values of box would be cleaner, even though they >> produce the exact same result. If we decide to obey box even with empty >> types, they don't. >> > > Or, if we decide to forbid 'box':true on an empty type, then this needs > tweaking anyway. > >>> { 'event': 'EVENT_B', >>> 'data': { } } >>> { 'event': 'EVENT_C', >>> 'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } } >>> { 'event': 'EVENT_D', >>> 'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } } >>> +{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' } >> >> This is case "struct". >> >> Missing: case "union". >>
diff --git a/scripts/qapi.py b/scripts/qapi.py index 108363d..6742e7a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -522,10 +522,14 @@ def check_type(expr_info, source, value, allow_array=False, def check_command(expr, expr_info): name = expr['command'] + box = expr.get('box', False) + args_meta = ['struct'] + if box: + args_meta += ['union'] check_type(expr_info, "'data' for command '%s'" % name, - expr.get('data'), allow_dict=True, allow_optional=True, - allow_metas=['struct']) + expr.get('data'), allow_dict=not box, allow_optional=True, + allow_metas=args_meta) returns_meta = ['union', 'struct'] if name in returns_whitelist: returns_meta += ['built-in', 'alternate', 'enum'] @@ -537,11 +541,15 @@ def check_command(expr, expr_info): def check_event(expr, expr_info): global events name = expr['event'] + box = expr.get('box', False) + meta = ['struct'] + if box: + meta += ['union'] events.append(name) check_type(expr_info, "'data' for event '%s'" % name, - expr.get('data'), allow_dict=True, allow_optional=True, - allow_metas=['struct']) + expr.get('data'), allow_dict=not box, allow_optional=True, + allow_metas=meta) def check_union(expr, expr_info): @@ -694,6 +702,10 @@ def check_keys(expr_elem, meta, required, optional=[]): raise QAPIExprError(info, "'%s' of %s '%s' should only use false value" % (key, meta, name)) + if key == 'box' and value is not True: + raise QAPIExprError(info, + "'%s' of %s '%s' should only use true value" + % (key, meta, name)) for key in required: if key not in expr: raise QAPIExprError(info, @@ -725,10 +737,10 @@ def check_exprs(exprs): add_struct(expr, info) elif 'command' in expr: check_keys(expr_elem, 'command', [], - ['data', 'returns', 'gen', 'success-response']) + ['data', 'returns', 'gen', 'success-response', 'box']) add_name(expr['command'], info, 'command') elif 'event' in expr: - check_keys(expr_elem, 'event', [], ['data']) + check_keys(expr_elem, 'event', [], ['data', 'box']) add_name(expr['event'], info, 'event') else: raise QAPIExprError(expr_elem['info'], @@ -1640,12 +1652,13 @@ extern const char *const %(c_name)s_lookup[]; def gen_params(arg_type, box, extra): - if not arg_type: + if not arg_type or arg_type.is_empty(): return extra ret = '' sep = '' if box: - assert False # not implemented + ret += '%s arg' % arg_type.c_param_type() + sep = ', ' else: for memb in arg_type.members: ret += sep diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 79d4eea..dc41fad 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -30,7 +30,8 @@ def gen_call(name, arg_type, box, ret_type): argstr = '' if box: - assert False # not implemented + if arg_type and not arg_type.is_empty(): + argstr = '&arg, ' elif arg_type: assert not arg_type.variants for memb in arg_type.members: diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index fd953fe..b8ca8c8 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -96,7 +96,10 @@ def gen_event_send(name, arg_type, box): ''') if box: - assert False # not implemented + ret += mcgen(''' + visit_type_%(c_name)s(v, NULL, &arg, &err); +''', + c_name=arg_type.c_name(), name=arg_type.name) else: ret += mcgen(''' visit_start_struct(v, "%(name)s", NULL, 0, &err); diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 87fc759..007db37 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -60,6 +60,18 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp) return arg; } +void qmp_boxed_empty(Error **errp) +{ +} + +void qmp_boxed_struct(UserDefZero *arg, Error **errp) +{ +} + +void qmp_boxed_union(UserDefNativeListUnion *arg, Error **errp) +{ +} + __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a, __org_qemu_x_StructList *b, __org_qemu_x_Union2 *c, diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index aefc29e..40f050e 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -410,7 +410,7 @@ following example objects: === Commands === Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, - '*returns': TYPE-NAME, + '*returns': TYPE-NAME, '*box': true, '*gen': false, '*success-response': false } Commands are defined by using a dictionary containing several members, @@ -461,6 +461,17 @@ which would validate this Client JSON Protocol transaction: => { "execute": "my-second-command" } <= { "return": [ { "value": "one" }, { } ] } +By default, the generator creates a marshalling function that converts +an input QDict into a function call implemented by the user, and +declares a prototype for the user's function which has a parameter for +each member of the argument struct, including boolean arguments that +describe whether optional arguments were provided. But if the QAPI +description includes the key 'box' with the boolean value true, the +user call prototype will have only a single parameter for the overall +generated C structure. The 'box' key is required in order to use a +union as an input argument, since it is not possible to list all +members of the union as separate parameters. + In rare cases, QAPI cannot express a type-safe representation of a corresponding Client JSON Protocol command. You then have to suppress generation of a marshalling function by including a key 'gen' with @@ -484,7 +495,8 @@ use of this member. === Events === -Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT } +Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, + '*box': true } Events are defined with the keyword 'event'. It is not allowed to name an event 'MAX', since the generator also produces a C enumeration @@ -505,6 +517,14 @@ Resulting in this JSON object: "data": { "b": "test string" }, "timestamp": { "seconds": 1267020223, "microseconds": 435656 } } +By default, the generator creates a C function that takes as +parameters each member of the argument struct and turns it into the +appropriate JSON Client event. But if the QAPI description includes +the key 'box' with the boolean value true, the event function will +have only a single parameter for the overall generated C structure. +The 'box' key is required in order to use a union as the data key, +since it is not possible to list all members of the union as separate +parameters. == Client JSON Protocol introspection == diff --git a/tests/Makefile b/tests/Makefile index c608f9a..5cd6177 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -272,6 +272,8 @@ qapi-schema += args-alternate.json qapi-schema += args-any.json qapi-schema += args-array-empty.json qapi-schema += args-array-unknown.json +qapi-schema += args-bad-box.json +qapi-schema += args-box-anon.json qapi-schema += args-int.json qapi-schema += args-invalid.json qapi-schema += args-member-array-bad.json diff --git a/tests/qapi-schema/args-bad-box.err b/tests/qapi-schema/args-bad-box.err new file mode 100644 index 0000000..16afe3c --- /dev/null +++ b/tests/qapi-schema/args-bad-box.err @@ -0,0 +1 @@ +tests/qapi-schema/args-bad-box.json:2: 'box' of command 'foo' should only use true value diff --git a/tests/qapi-schema/args-bad-box.exit b/tests/qapi-schema/args-bad-box.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/args-bad-box.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/args-bad-box.json b/tests/qapi-schema/args-bad-box.json new file mode 100644 index 0000000..8d5737a --- /dev/null +++ b/tests/qapi-schema/args-bad-box.json @@ -0,0 +1,2 @@ +# 'box' should only appear with value true +{ 'command': 'foo', 'box': false } diff --git a/tests/qapi-schema/args-bad-box.out b/tests/qapi-schema/args-bad-box.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/args-box-anon.err b/tests/qapi-schema/args-box-anon.err new file mode 100644 index 0000000..11eaefc --- /dev/null +++ b/tests/qapi-schema/args-box-anon.err @@ -0,0 +1 @@ +tests/qapi-schema/args-box-anon.json:2: 'data' for command 'foo' should be a type name diff --git a/tests/qapi-schema/args-box-anon.exit b/tests/qapi-schema/args-box-anon.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/args-box-anon.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/args-box-anon.json b/tests/qapi-schema/args-box-anon.json new file mode 100644 index 0000000..947e3c6 --- /dev/null +++ b/tests/qapi-schema/args-box-anon.json @@ -0,0 +1,2 @@ +# 'box' can only be used with named types +{ 'command': 'foo', 'box': true, 'data': { 'string': 'str' } } diff --git a/tests/qapi-schema/args-box-anon.out b/tests/qapi-schema/args-box-anon.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err index 1d693d7..f8ad223 100644 --- a/tests/qapi-schema/args-union.err +++ b/tests/qapi-schema/args-union.err @@ -1 +1 @@ -tests/qapi-schema/args-union.json:4: 'data' for command 'oops' cannot use union type 'Uni' +tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use union type 'Uni' diff --git a/tests/qapi-schema/args-union.json b/tests/qapi-schema/args-union.json index 7bdcbb7..c0ce091 100644 --- a/tests/qapi-schema/args-union.json +++ b/tests/qapi-schema/args-union.json @@ -1,4 +1,3 @@ -# we do not allow union arguments -# TODO should we support this? +# use of union arguments requires 'box':true { 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } } { 'command': 'oops', 'data': 'Uni' } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index f571e1b..df91f3d 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -127,6 +127,9 @@ { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' }, 'returns': 'int' } { 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' } +{ 'command': 'boxed-empty', 'box': true } +{ 'command': 'boxed-struct', 'box': true, 'data': 'UserDefZero' } +{ 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'box': true } # For testing integer range flattening in opts-visitor. The following schema # corresponds to the option format: @@ -147,13 +150,14 @@ { 'struct': 'EventStructOne', 'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } } -{ 'event': 'EVENT_A' } +{ 'event': 'EVENT_A', 'box': true } { 'event': 'EVENT_B', 'data': { } } { 'event': 'EVENT_C', 'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } } { 'event': 'EVENT_D', 'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } } +{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' } # test that we correctly compile downstream extensions, as well as munge # ticklish names diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 754a926..8a00c6b 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -23,13 +23,15 @@ alternate AltStrNum case s: str case n: number event EVENT_A None - box=False + box=True event EVENT_B None box=False event EVENT_C q_obj_EVENT_C-arg box=False event EVENT_D q_obj_EVENT_D-arg box=False +event EVENT_E UserDefZero + box=True object Empty1 object Empty2 base Empty1 @@ -153,6 +155,12 @@ object __org.qemu_x-Union2 case __org.qemu_x-value: __org.qemu_x-Struct2 command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1 gen=True success_response=True box=False +command boxed-empty None -> None + gen=True success_response=True box=True +command boxed-struct UserDefZero -> None + gen=True success_response=True box=True +command boxed-union UserDefNativeListUnion -> None + gen=True success_response=True box=True command guest-get-time q_obj_guest-get-time-arg -> int gen=True success_response=True box=False command guest-sync q_obj_guest-sync-arg -> any
Turn on the ability to pass command and event arguments in a single boxed parameter. For structs, it makes it possible to pass a single qapi type instead of a breakout of all struct members; for unions, it is now possible to use a union as the data for a command or event. Generated code is unchanged, as long as no client uses the new feature. Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: rebase to latest v6: retitle, rebase, and merge v5 40/46 and 41/46 into one patch --- scripts/qapi.py | 29 +++++++++++++++++++++-------- scripts/qapi-commands.py | 3 ++- scripts/qapi-event.py | 5 ++++- tests/test-qmp-commands.c | 12 ++++++++++++ docs/qapi-code-gen.txt | 24 ++++++++++++++++++++++-- tests/Makefile | 2 ++ tests/qapi-schema/args-bad-box.err | 1 + tests/qapi-schema/args-bad-box.exit | 1 + tests/qapi-schema/args-bad-box.json | 2 ++ tests/qapi-schema/args-bad-box.out | 0 tests/qapi-schema/args-box-anon.err | 1 + tests/qapi-schema/args-box-anon.exit | 1 + tests/qapi-schema/args-box-anon.json | 2 ++ tests/qapi-schema/args-box-anon.out | 0 tests/qapi-schema/args-union.err | 2 +- tests/qapi-schema/args-union.json | 3 +-- tests/qapi-schema/qapi-schema-test.json | 6 +++++- tests/qapi-schema/qapi-schema-test.out | 10 +++++++++- 18 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 tests/qapi-schema/args-bad-box.err create mode 100644 tests/qapi-schema/args-bad-box.exit create mode 100644 tests/qapi-schema/args-bad-box.json create mode 100644 tests/qapi-schema/args-bad-box.out create mode 100644 tests/qapi-schema/args-box-anon.err create mode 100644 tests/qapi-schema/args-box-anon.exit create mode 100644 tests/qapi-schema/args-box-anon.json create mode 100644 tests/qapi-schema/args-box-anon.out