From patchwork Fri Jul 13 22:48:58 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 170981 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id C86672C02FD for ; Sat, 14 Jul 2012 08:48:06 +1000 (EST) Received: from localhost ([::1]:48238 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Spoea-0004Ao-I5 for incoming@patchwork.ozlabs.org; Fri, 13 Jul 2012 18:48:04 -0400 Received: from eggs.gnu.org ([208.118.235.92]:47517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpoeR-0004Af-9F for qemu-devel@nongnu.org; Fri, 13 Jul 2012 18:47:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SpoeP-0005pu-Ez for qemu-devel@nongnu.org; Fri, 13 Jul 2012 18:47:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpoeP-0005pp-38 for qemu-devel@nongnu.org; Fri, 13 Jul 2012 18:47:53 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6DMlqXs004106 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Jul 2012 18:47:52 -0400 Received: from lacos-laptop.usersys.redhat.com (vpn1-6-102.ams2.redhat.com [10.36.6.102]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q6DMljAa007543; Fri, 13 Jul 2012 18:47:45 -0400 Message-ID: <5000A5DA.9050904@redhat.com> Date: Sat, 14 Jul 2012 00:48:58 +0200 From: Laszlo Ersek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.5) Gecko/20120601 Thunderbird/10.0.5 MIME-Version: 1.0 To: Luiz Capitulino References: <1339575768-2557-1-git-send-email-lersek@redhat.com> <1339575768-2557-6-git-send-email-lersek@redhat.com> <20120713135136.049b9e54@doriath.home> In-Reply-To: <20120713135136.049b9e54@doriath.home> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Paolo Bonzini , qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 07/13/12 18:51, Luiz Capitulino wrote: > On Wed, 13 Jun 2012 10:22:36 +0200 > Laszlo Ersek wrote: >> Repeating an optarg is supported; > > I see that the current code supports this too, but why? Something > like this should fail: > > -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch > Also, you're using a queue to support the repeating of optargs, > right? I think this could be simplified if we just don't support > that. I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd depend on repetition. When the outermost opts_start_struct() is invoked and I shovel the optargs into the queues, I can't yet know what's going to be used in repeated form and what not. If you prefer I can change lookup_scalar() as follows. For reference: >> +static GQueue * >> +lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) >> +{ >> + GQueue *list; >> + >> + list = g_hash_table_lookup(ov->unprocessed_opts, name); >> + if (!list) { >> + error_set(errp, QERR_MISSING_PARAMETER, name); >> + } >> + return list; >> +} >> +static void >> +opts_start_optional(Visitor *v, bool *present, const char *name, >> + Error **errp) >> +{ >> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); >> + >> + /* we only support a single mandatory scalar field in a list node */ >> + assert(ov->repeated_opts == NULL); >> + *present = (lookup_distinct(ov, name, NULL) != NULL); >> +} >> +static const QemuOpt * >> +lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp) >> +{ >> + if (ov->repeated_opts == NULL) { >> + GQueue *list; >> + >> + /* the last occurrence of any QemuOpt takes effect when queried by name >> + */ >> + list = lookup_distinct(ov, name, errp; >> + return list ? g_queue_peek_tail(list) : NULL; We're outside of list traversal in this branch, meaning the optarg is allowed exactly once. (Optional optargs are first handled by opts_start_optional().) If lookup_distinct() succeeds here, then rather than returning the last occurrence, I could check the depth of the queue (== 1 or > 1), and set an error for > 1. However QemuOpts definitely supports repeated optargs now (otherwise slirp hostfwd/guestfwd wouldn't work). qemu_opt_foreach() is used for iteration (with QTAILQ_FOREACH()), while qemu_opt_find() -- and thus its direct callers -- rely on QTAILQ_FOREACH_REVERSE() and the first match. Optargs of an option are apparently chained like this: qemu_opts_parse() [qemu-option.c] opts_parse(..., defaults=false) opts_do_parse(..., prepend=false) opt_set(..., prepend=false, ...) QTAILQ_INSERT_TAIL() "-option arg=val1,arg=val2,arg=val3" is therefore linked into the corresponding QemuOpts instance in the same order, and qemu_opt_find() will return "arg=val3". I also use g_queue_push_tail() and g_queue_peek_tail(), so I think we're compatible. >> + } >> + return g_queue_peek_head(ov->repeated_opts); >> +} Continuing slightly out of order: >> +/* mimics qemu-option.c::parse_option_bool() */ >> +static void >> +opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) >> +{ >> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); >> + const QemuOpt *opt; >> + >> + opt = lookup_scalar(ov, name, errp); >> + if (!opt) { >> + return; >> + } >> + >> + if (opt->str) { >> + if (strcmp(opt->str, "on") == 0 || >> + strcmp(opt->str, "yes") == 0 || >> + strcmp(opt->str, "y") == 0) { >> + *obj = true; >> + } else if (strcmp(opt->str, "off") == 0 || >> + strcmp(opt->str, "no") == 0 || >> + strcmp(opt->str, "n") == 0) { >> + *obj = false; > > The current code only accepts 'on' or 'off', no reason to change that. > >> + } else { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, >> + "on|yes|y|off|no|n"); >> + return; >> + } >> + } else { >> + *obj = true; >> + } >> + >> + processed(ov, name); >> +} This function is used for "bool" generally. The following optargs were all unified as "bool": - slirp/restrict: originally QEMU_OPT_STRING, net_init_slirp() accepting all of "on|yes|y|off|no|n" - tap/vnet_hdr: originally QEMU_OPT_BOOL, parse_option_bool() accepting "on|off". - tap/vhost: ditto - tap/vhostforce: ditto So I took the union (nothing should break that used to work). The leading comment rather means that the structure of parse_option_bool() is followed: - optarg values meaning "true": true - optarg values meaning "false": false - other optarg values: error - no optarg value at all: true >> +static void >> +opts_start_struct(Visitor *v, void **obj, const char *kind, >> + const char *name, size_t size, Error **errp) >> +{ >> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); >> + const QemuOpt *opt; >> + >> + *obj = g_malloc0(size); >> + if (ov->depth++ > 0) { >> + return; >> + } >> + >> + ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, >> + NULL, &destroy_list); >> + QTAILQ_FOREACH(opt, &ov->opts_root->head, next) { >> + GQueue *list; >> + >> + list = g_hash_table_lookup(ov->unprocessed_opts, opt->name); >> + if (list == NULL) { >> + list = g_queue_new(); >> + >> + /* GHashTable will never try to free the keys -- we supplied NULL >> + * as "key_destroy_func" above. Thus cast away key const-ness in >> + * order to suppress gcc's warning. */ >> + g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name, >> + list); >> + } >> + >> + /* Similarly, destroy_list() doesn't call g_queue_free_full(). */ >> + g_queue_push_tail(list, (gpointer)opt); >> + } >> +} > > This doesn't insert the opts id into the hash, so opts_type_str() > will fail to find the id when the generated code visits it here: > > void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error **errp) > { > if (!error_is_set(errp)) { > Error *err = NULL; > visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), &err); > if (!err) { > assert(!obj || *obj); > visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <--------- > [...] > *groan* You're right. opts_do_parse() makes an exception with "id" and doesn't call opt_set() for any occurrence of it. Would you accept the attached fix, split up and squashed into previous parts appropriately? Thanks! Laszlo From 6839ead5ac4a77ac82aee2fe1365e72e276aa89d Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Fri, 13 Jul 2012 23:49:09 +0200 Subject: [PATCH] support NetLegacy::id and clean up QemuOpts::id usage NetLegacy::id is actually allowed and takes precedence over NetLegacy::name. Factor opts_visitor_insert() out of opts_start_struct() and call it separately for opts_root->id if there's any. Signed-off-by: Laszlo Ersek --- net.c | 2 +- qapi-schema.json | 5 ++++- qapi/opts-visitor.c | 49 +++++++++++++++++++++++++++++++++++++------------ 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/net.c b/net.c index 1612f64..dbca77b 100644 --- a/net.c +++ b/net.c @@ -869,7 +869,7 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) u.net = object; opts = u.net->opts; /* missing optional values have been initialized to "all bits zero" */ - name = u.net->name; + name = u.net->has_id ? u.net->id : u.net->name; } if (net_client_init_fun[opts->kind]) { diff --git a/qapi-schema.json b/qapi-schema.json index ed345ee..cc48127 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2116,7 +2116,9 @@ # # @vlan: #optional vlan number # -# @name: #optional identifier for monitor commands +# @id: #optional identifier for monitor commands +# +# @name: #optional identifier for monitor commands, ignored if @id is present # # @traits: device type specific properties (legacy) # @@ -2125,6 +2127,7 @@ { 'type': 'NetLegacy', 'data': { '*vlan': 'int32', + '*id': 'str', '*name': 'str', 'opts': 'NetClientOptions' } } diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 9187c86..a261cf3 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -35,6 +35,12 @@ struct OptsVisitor * schema, with a single mandatory scalar member. */ GQueue *repeated_opts; bool repeated_opts_first; + + /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for + * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does + * not survive or escape the OptsVisitor object. + */ + QemuOpt *fake_id_opt; }; @@ -46,6 +52,27 @@ destroy_list(gpointer list) static void +opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) +{ + GQueue *list; + + list = g_hash_table_lookup(unprocessed_opts, opt->name); + if (list == NULL) { + list = g_queue_new(); + + /* GHashTable will never try to free the keys -- we supply NULL as + * "key_destroy_func" in opts_start_struct(). Thus cast away key + * const-ness in order to suppress gcc's warning. + */ + g_hash_table_insert(unprocessed_opts, (gpointer)opt->name, list); + } + + /* Similarly, destroy_list() doesn't call g_queue_free_full(). */ + g_queue_push_tail(list, (gpointer)opt); +} + + +static void opts_start_struct(Visitor *v, void **obj, const char *kind, const char *name, size_t size, Error **errp) { @@ -60,21 +87,18 @@ opts_start_struct(Visitor *v, void **obj, const char *kind, ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, NULL, &destroy_list); QTAILQ_FOREACH(opt, &ov->opts_root->head, next) { - GQueue *list; + /* ensured by qemu-option.c::opts_do_parse() */ + assert(strcmp(opt->name, "id") != 0); - list = g_hash_table_lookup(ov->unprocessed_opts, opt->name); - if (list == NULL) { - list = g_queue_new(); + opts_visitor_insert(ov->unprocessed_opts, opt); + } - /* GHashTable will never try to free the keys -- we supplied NULL - * as "key_destroy_func" above. Thus cast away key const-ness in - * order to suppress gcc's warning. */ - g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name, - list); - } + if (ov->opts_root->id != NULL) { + ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt); - /* Similarly, destroy_list() doesn't call g_queue_free_full(). */ - g_queue_push_tail(list, (gpointer)opt); + ov->fake_id_opt->name = "id"; + ov->fake_id_opt->str = ov->opts_root->id; + opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); } } @@ -390,6 +414,7 @@ opts_visitor_cleanup(OptsVisitor *ov) if (ov->unprocessed_opts != NULL) { g_hash_table_destroy(ov->unprocessed_opts); } + g_free(ov->fake_id_opt); memset(ov, '\0', sizeof *ov); }