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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4629 bytes --]

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    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]