Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Anthony Liguori]

On 03/29/2012 02:28 PM, Michael Roth wrote:
> On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote:
> > On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> > > This allows for QAPI functions to receive a variable-length argument
> > > list. This is going to be used by device_add and netdev_add commands.
> > >
> > > In the schema, the argument list is represented by type name '**',
> > > like this example:
> > >
> > >      { 'command': 'foo', 'data': { 'arg-list': '**' } }
> > >
> > > Each argument is represented by the KeyValues type and the C
> > > implementation should expect a KeyValuesList, like:
> > >
> > >      void qmp_foo(KeyValuesList *values_list, Error **errp);
> > >
> > > XXX: This implementation is simple but very hacky. We just iterate
> > >       through all arguments and build the KeyValuesList list to be
> > >       passed to the QAPI function.
> > >
> > >       Maybe we could have a kwargs type, that does exactly this but
> > >       through a visitor instead?
> > >
> > > Signed-off-by: Luiz Capitulino<address@hidden>
> >
> > What about just treating '**' as "marshal remaining arguments to a
> > string" and then pass that string to device_add?  qmp_device_add can
> > then parse that string with QemuOpts.
>
> Since currently we explicitly point qmp to the marshaller anyway, we
> could also just treat '**' as an indicator to not generate a marshaller.
> Then, we open-code the marshaller to process the QDict, rather than embedding
> it in the script or passing it through to qmp_device_add().

You could also just do gen=False...

But I don't think open coding the marshaller is the right thing here.  You have 
to convert to strings and reparse anyway.  The code needs to be shared between 
device_add and netdev_add too.

Regards,

Anthony Liguori

> > From the perspective of qmp_device_add() it then just looks like any
> other qmp command.
>
> > It's a bit ugly, but that's how things worked.  When we introduce
> > qom_add, this problem goes away because you would make multiple
> > calls to qom_set to set all of the properties.
> >
> > Regards,
> >
> > Anthony Liguori
> >
> > > ---
> > >   qapi-schema.json         |   15 +++++++++++++++
> > >   scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
> > >   scripts/qapi.py          |    2 ++
> > >   3 files changed, 45 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 0d11d6e..25bd487 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -1701,3 +1701,18 @@
> > >   # Since: 1.1
> > >   ##
> > >   { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> > > +
> > > +##
> > > +# @KeyValues:
> > > +#
> > > +# A generic representation of a key value pair.
> > > +#
> > > +# @key: the name of the item
> > > +#
> > > +# @value: the string representation of the item value.  This typically follows
> > > +#         QEMU's command line parsing format.  See the man pages for more
> > > +#         information.
> > > +#
> > > +# Since: 0.14.0
> > > +##
> > > +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
> > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > > index 30a24d2..75a6e81 100644
> > > --- a/scripts/qapi-commands.py
> > > +++ b/scripts/qapi-commands.py
> > > @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
> > >                        obj=obj)
> > >
> > >       for argname, argtype, optional, structured in parse_args(args):
> > > -        if optional:
> > > +        if optional and not '**':
> > >               ret += mcgen('''
> > >   visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
> > >   if (has_%(c_name)s) {
> > >   ''',
> > >                            c_name=c_var(argname), name=argname)
> > >               push_indent()
> > > -        ret += mcgen('''
> > > +        if argtype == '**':
> > > +            if dealloc:
> > > +                ret += mcgen('''
> > > +qapi_free_KeyValuesList(%(obj)s);
> > > +''',
> > > +                        obj=c_var(argname))
> > > +            else:
> > > +                ret += mcgen('''
> > > +{
> > > +    const QDictEntry *entry;
> > > +    v = v; /* fix me baby */
> > > +
> > > +    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
> > > +        KeyValuesList *item = g_malloc0(sizeof(*item));
> > > +        item->value = g_malloc0(sizeof(*item->value));
> > > +        item->value->key = g_strdup(qdict_entry_key(entry));
> > > +        item->value->value = 
> > > g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
> > > +
> > > +        item->next = %(obj)s;
> > > +        %(obj)s = item;
> > > +    }
> > > +}
> > > +''',
> > > +                        obj=c_var(argname))
> > > +        else:
> > > +            ret += mcgen('''
> > >   %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
> > >   ''',
> > >                        c_name=c_var(argname), name=argname, argtype=argtype,
> > >                        visitor=type_visitor(argtype))
> > > -        if optional:
> > > +        if optional and not '**':
> > >               pop_indent()
> > >               ret += mcgen('''
> > >   }
> > > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > > index e062336..87b9ee6 100644
> > > --- a/scripts/qapi.py
> > > +++ b/scripts/qapi.py
> > > @@ -163,6 +163,8 @@ def c_type(name):
> > >           return 'bool'
> > >       elif name == 'number':
> > >           return 'double'
> > > +    elif name == '**':
> > > +        return 'KeyValuesList *'
> > >       elif type(name) == list:
> > >           return '%s *' % c_list_type(name[0])
> > >       elif is_enum(name):