From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDLXj-0007Nm-5F for qemu-devel@nongnu.org; Thu, 29 Mar 2012 16:02:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SDLXg-0003ru-O1 for qemu-devel@nongnu.org; Thu, 29 Mar 2012 16:01:58 -0400 Received: from e6.ny.us.ibm.com ([32.97.182.146]:53268) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDLXg-0003pn-Jb for qemu-devel@nongnu.org; Thu, 29 Mar 2012 16:01:56 -0400 Received: from /spool/local by e6.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 29 Mar 2012 16:01:47 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 266286E804C for ; Thu, 29 Mar 2012 16:01:27 -0400 (EDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q2TK1QE9306178 for ; Thu, 29 Mar 2012 16:01:26 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q2TK1Gax010478 for ; Thu, 29 Mar 2012 14:01:17 -0600 Message-ID: <4F74BF8C.3070800@us.ibm.com> Date: Thu, 29 Mar 2012 15:01:16 -0500 From: Anthony Liguori MIME-Version: 1.0 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> In-Reply-To: <20120329192813.GA14158@illuin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Michael Roth Cc: armbru@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, Luiz Capitulino 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... 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): >> >