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<lcapitul...@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):
> > >>
> > >
> >
