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

[Eric Blake]

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.

>  #
> -# 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.

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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature