From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53584) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDOGu-0007Wb-Ub for qemu-devel@nongnu.org; Thu, 29 Mar 2012 18:56:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SDOGr-0004mV-RS for qemu-devel@nongnu.org; Thu, 29 Mar 2012 18:56:48 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:46656) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDOGr-0004lY-HT for qemu-devel@nongnu.org; Thu, 29 Mar 2012 18:56:45 -0400 Received: by obbwd20 with SMTP id wd20so91954obb.4 for ; Thu, 29 Mar 2012 15:56:43 -0700 (PDT) Sender: fluxion Date: Thu, 29 Mar 2012 17:56:38 -0500 From: Michael Roth Message-ID: <20120329225638.GB22887@illuin> References: <1333042003-15490-1-git-send-email-lcapitulino@redhat.com> <1333042003-15490-13-git-send-email-lcapitulino@redhat.com> <4F74A95A.7050403@us.ibm.com> <20120329192813.GA14158@illuin> <4F74BF8C.3070800@us.ibm.com> <20120329223910.GA22887@illuin> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120329223910.GA22887@illuin> Subject: Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: armbru@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, Luiz Capitulino 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 > > >> > > >>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): > > >> > > > > >