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

[Michael Roth]

On Thu, Mar 29, 2012 at 03:01:16PM -0500, Anthony Liguori wrote:
> 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...

Ahh, yes we could. Nice :)

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

The only thing the marshallers need to do is call qemu_opts_from_qdict()
and pass them on to qdev_device_add()/net_client_init()/etc. We'd basically
be taking the current qmp implementations and modifying their call signatures
to be compatible with qmp-dispatch.c in the future. It's not really
qapi-ish, admittedly, but neither is a built-in varargs sort of type.

I'd just prefer not to bake legacy hooks into the code generators if we
don't have to. If we absolutely have to do this in the future, it would be more
sense to define such fields as being string arguments from the get-go.

> 
> 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):
> >>
> >
>