Re: [Qemu-devel] [PATCH RFC v2 42/47] qapi-schema: Fix up misleading specification of netdev_add

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> It doesn't take a 'props' argument, let alone one in the format
>> "NAME=VALUE,..."
>> 
>> The bogus arguments specification doesn't matter due to 'gen': false.
>> Clean it up to be incomplete rather than wrong, and document the
>> incompleteness.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  docs/qapi-code-gen.txt |  2 +-
>>  qapi-schema.json       | 13 +++++++------
>>  2 files changed, 8 insertions(+), 7 deletions(-)
>> 
>
>> +++ b/qapi-schema.json
>> @@ -2062,11 +2062,12 @@
>>  #
>>  # @id: the name of the new network backend
>>  #
>> -# @props: #optional a list of properties to be passed to the backend in
>> -#         the format 'name=value', like 'ifname=tap0,script=no'
>> +# Additional arguments depend on the type.
>
> In other words, this would be a perfect candidate to write as taking a
> flat union as the argument, where 'type' is the discriminator, and where
> the additional arguments are branches of the union.  Except that we
> don't yet support passing a union as the direct "arguments":{...} of a
> QMP call, so we still have some design work to do if we want to go
> there.  Later, of course.

Exactly.  I started hacking in that direction, but had to stop to get
this stuff out.

>>  #
>> -# Notes: The semantics of @props is not well defined.  Future
>> commands will be
>> -#        introduced that provide stronger typing for backend creation.
>> +# TODO This command effectively bypasses QAPI completely due to its
>> +# "additional arguments" business.  It shouldn't have been added to
>> +# the schema in this form.  It should be qapified properly, or
>> +# replaced by a properly qapified command.
>
> I like that you are getting rid of the sentence about strong typing
> (since flat unions ARE strongly typed) and instead couching it in terms
> of an incomplete qapi description.  I think we may even be able to come
> up with a way to express it without needing a new command, and without
> breaking back-compat, once we figure out how to pass unions as a command
> argument.
>
> In fact, let's suppose we do have a new union type, NetdevInfo, as well
> as a way to flag that a given command will be called by the marshaler
> using JUST a pointer to the struct, rather than as a list of parameters
> to members of the struct.
>
> { 'enum': 'NetdevTypes', [ ... ] }
> { 'struct': 'NetdevInfoBase', 'data':
>    { 'type': 'NetdevTypes', 'id': 'str' } }
> { 'union': 'NetdevInfo', 'base': 'NetdevInfoBase',
>   'discriminator': 'type', 'data': { ... } }
> { 'command': 'netdev_add', 'boxed': true,
>   'data': 'NetdevInfo' }
>
> resulting in the marshaller calling
>
> void qmp_netdev_add(NetdevInfo *data, Error **errp)
>
>
> Here, 'boxed' is an optional key to each 'command', defaulting to false;
> but when true, 'data' is passed in a box to the command called by the
> marshaler, instead of broken out into fields. 'boxed' must be true for
> unions, but may be true elsewhere.

Matches my plans.

Note existing type NetClientOptions, used internally.  I hope we can
avoid duplicating it and all its Netdev*Options types.

> And if we have that, then OTHER commands that have complained about
> super-long parameter lists when their struct is exploded into a
> parameter list could also use this mechanism, to write their handler in
> terms of a single pointer to the struct.

Yes.

> Enough rambling about stuff for the future. This patch is fine as-is;
> Reviewed-by: Eric Blake <address@hidden>

Thanks!