From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpFA4-00050Y-Of for qemu-devel@nongnu.org; Wed, 28 Sep 2016 09:44:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpF9z-0001a3-9h for qemu-devel@nongnu.org; Wed, 28 Sep 2016 09:44:35 -0400 Date: Wed, 28 Sep 2016 14:44:11 +0100 From: "Daniel P. Berrange" Message-ID: <20160928134411.GD15510@redhat.com> Reply-To: "Daniel P. Berrange" References: <1474982001-20878-1-git-send-email-berrange@redhat.com> <1474982001-20878-4-git-send-email-berrange@redhat.com> <8f143756-3a86-66d3-1171-f19dcc5eb02c@redhat.com> <20160928093505.GI21583@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160928093505.GI21583@redhat.com> Subject: Re: [Qemu-devel] [PATCH v14 03/19] option: allow qemu_opts_to_qdict to merge repeated options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-block@nongnu.org, Markus Armbruster , qemu-devel@nongnu.org, Max Reitz , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= On Wed, Sep 28, 2016 at 10:35:05AM +0100, Daniel P. Berrange wrote: > On Tue, Sep 27, 2016 at 05:03:17PM -0500, Eric Blake wrote: > > On 09/27/2016 08:13 AM, Daniel P. Berrange wrote: > > > If given an option string such as > > > > > > size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind > > > > > > the qemu_opts_to_qdict() method will currently overwrite > > > the values for repeated option keys, so only the last > > > value is in the returned dict: > > > > > > size=1024 > > > nodes=1-2 > > > policy=bind > > > > > > This adds the ability for the caller to ask that the > > > repeated keys be turned into list indexes: > > > > > > size=1024 > > > nodes.0=10 > > > nodes.1=4-5 > > > nodes.2=1-2 > > > policy=bind > > > > > > Note that the conversion has no way of knowing whether > > > any given key is expected to be a list upfront - it can > > > only figure that out when seeing the first duplicated > > > key. Thus the caller has to be prepared to deal with the > > > fact that if a key 'foo' is a list, then the returned > > > qdict may contain either 'foo' (if only a single instance > > > of the key was seen) or 'foo.NN' (if multiple instances > > > of the key were seen). > > > > > > Signed-off-by: Daniel P. Berrange > > > --- > > > > If I'm not mistaken, this policy adds a new policy, but all existing > > clients use the old policy, and the new policy is exercised only by the > > testsuite additions. Might be useful to mention that in the commit > > message, rather than making me read the whole commit before guessing that. > > Correct, this will only be used in combination with the > later qdict_crumple method. > > > > +++ b/include/qemu/option.h > > > @@ -125,7 +125,13 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params, > > > int permit_abbrev); > > > QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict, > > > Error **errp); > > > -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict); > > > +typedef enum { > > > + QEMU_OPTS_REPEAT_POLICY_LAST, > > > + QEMU_OPTS_REPEAT_POLICY_LIST, > > > > Hmm. I suspect this subtle difference (one vowel) to be the source of > > typo bugs. Can we come up with more obvious policy names, such as > > LAST_ONLY vs. INTO_LIST? Except that doing that makes it harder to fit > > 80 columns. So up to you if you want to ignore me here. > > I'll use POLICY_LAST and POLICY_ALL. > > > ERROR: an occurrence of a duplicate option is considered an error > > Yeah, sure can add that easily. > > > Also, while you turn 'foo=a,foo=b' into 'foo.0=a,foo.1=b', does your > > code correctly handle the cases of 'foo.0=a,foo=b' and 'foo=a,foo.1=b'? > > (And what IS the correct handling of those cases logically supposed to be?) > > I'd be inclined to say that is an error. We're only doing this > hack to maintain compatibility with existing OptsVisitor > semantics for repeated options, and the scenario you illustrate > is not possible with OptsVisitor. So I say we keep it an error. > Either use the old syntax or the new syntax, but not mix them > ever. Having looked at this again, I don't think this code should try to detect 'foo.0=a,foo=b', because qemu_opts_to_qdict() is not placing an semantic interpretation on the keys. Such semantics are defined by the qdict_crumple() method, so that's where I'll have to do such error detection. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|