From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxETD-00085e-2K for qemu-devel@nongnu.org; Thu, 20 Oct 2016 10:37:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxET9-0007rh-UL for qemu-devel@nongnu.org; Thu, 20 Oct 2016 10:37:23 -0400 Date: Thu, 20 Oct 2016 15:37:04 +0100 From: "Daniel P. Berrange" Message-ID: <20161020143704.GE27909@redhat.com> Reply-To: "Daniel P. Berrange" References: <1475246744-29302-1-git-send-email-berrange@redhat.com> <1475246744-29302-13-git-send-email-berrange@redhat.com> <87wphcd506.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87wphcd506.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= On Thu, Oct 13, 2016 at 10:31:37AM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" writes: > > > 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=QString("1024") > > nodes=QString("1-2") > > policy=QString("bind") > > > > With this change the caller can optionally ask for all > > the repeated values to be stored in a QList. In the > > above example that would result in 'nodes' being a > > QList, so the returned dict would contain > > > > size=QString("1024") > > nodes=QList([QString("10"), > > QString("4-5"), > > QString("1-2")]) > > policy=QString("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 a QString or a QList for the > > key 'foo'. > > Actually three cases, not two: > > 0. qdict does not contain the key means empty list. > > 1. qdict contains the key with a QString value means list of one > element. > > 2. qdict contains the key with a QList value means list of more than one > element. > > I consider this weird. However, it's usefully weird with at least your > QObject input visitor. > > > In a third mode, it is possible to ask for repeated > > options to be reported as an error, rather than silently > > dropping all but the last one. > > Got users for this policy in the pipeline? I in fact used it in the QObjectInputVisitor, when the autocreate_list is not set. I guess strictly speaking this is not back-compatible if someone is passing repeated keys, but I judged that rather than silently ignoring this incorrect usage, it was acceptable to report an error. > > QTAILQ_FOREACH(opt, &opts->head, next) { > > val = QOBJECT(qstring_from_str(opt->str)); > > - qdict_put_obj(qdict, opt->name, val); > > + > > + if (qdict_haskey(ret, opt->name)) { > > + switch (repeatPolicy) { > > + case QEMU_OPTS_REPEAT_POLICY_ERROR: > > + error_setg(errp, "Option '%s' appears more than once", > > + opt->name); > > + qobject_decref(val); > > + if (!qdict) { > > + QDECREF(ret); > > + } > > + return NULL; > > + > > + case QEMU_OPTS_REPEAT_POLICY_ALL: > > + prevval = qdict_get(ret, opt->name); > > + if (qobject_type(prevval) == QTYPE_QLIST) { > > + /* Already identified this key as a list */ > > + list = qobject_to_qlist(prevval); > > + } else { > > + /* Replace current scalar with a list */ > > + list = qlist_new(); > > + qobject_incref(prevval); > > Where is this reference decremented? 'prevval' is a borrowed reference from 'ret', against the key opt->name. qdict_put_obj decrements the reference we borrowed from ret against the key opt->name. qlist_append_obj() takes ownership of the reference it is passed, so we must qobject_incref() to avoid qdict_put_obj free'ing prevval. When we call qdict_put_obj() we're replacing the value currently associted > > > + qlist_append_obj(list, prevval); > > + qdict_put_obj(ret, opt->name, QOBJECT(list)); > > + } > > + qlist_append_obj(list, val); > > + break; > > + > > + case QEMU_OPTS_REPEAT_POLICY_LAST: > > + /* Just discard previously set value */ > > + qdict_put_obj(ret, opt->name, val); > > + break; > > + } > > + } else { > > + qdict_put_obj(ret, opt->name, val); > > + } > > A possible alternative: > > val = QOBJECT(qstring_from_str(opt->str)); > > if (qdict_haskey(ret, opt->name)) { > switch (repeatPolicy) { > case QEMU_OPTS_REPEAT_POLICY_ERROR: > error_setg(errp, "Option '%s' appears more than once", > opt->name); > qobject_decref(val); > if (!qdict) { > QDECREF(ret); > } > return NULL; > > case QEMU_OPTS_REPEAT_POLICY_ALL: > prevval = qdict_get(ret, opt->name); > if (qobject_type(prevval) == QTYPE_QLIST) { > /* Already identified this key as a list */ > list = qobject_to_qlist(prevval); > } else { > /* Replace current scalar with a list */ > list = qlist_new(); > qobject_incref(prevval); > qlist_append_obj(list, prevval); > } > qlist_append_obj(list, val); > val = QOBJECT(list); > break; > > case QEMU_OPTS_REPEAT_POLICY_LAST: > break; > } > } > qdict_put_obj(ret, opt->name, val); > > This shows the common part of the behavior more clearly. Matter of > taste, you get to use your artistic license. 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/ :|