Eric Blake <ebl...@redhat.com> writes:
> 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.
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".
>>
