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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Thu, Mar 29, 2012 at 05:39:10PM -0500, Michael Roth wrote:
> 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<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...
> 
> 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

Err, scratch that... I've been living in qemu-ga land too long. For
QMP this basically amounts to a no-op for now (other than documenting the
command in the schema and doing the error-handling cleanups), and when we
switch to the new qmp server/dispatch stuff we just massage the function
signature to match the marshaller qapi expects and continue to skip
marshaller generation.

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