Re: [Qemu-devel] [PATCH v7 07/15] qapi: Implement boxed types for commands/events

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/14/2016 09:27 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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".
>>