On 06/14/2016 09:27 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
