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

[Anthony Liguori <aliguori@us.ibm.com>]

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<lcapitulino@redhat.com>
>>
>> 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):
>>
>