From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjl1Q-0004Hk-1u for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:33:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjl1O-00058U-JA for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:33:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55664) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjl1O-000586-Bh for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:32:58 -0400 Date: Tue, 13 Sep 2016 11:32:54 +0100 From: "Daniel P. Berrange" Message-ID: <20160913103253.GQ30949@redhat.com> Reply-To: "Daniel P. Berrange" References: <1473088600-17930-1-git-send-email-berrange@redhat.com> <1473088600-17930-7-git-send-email-berrange@redhat.com> <83dfd165-7ae5-ed42-bd8d-29af0095460a@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <83dfd165-7ae5-ed42-bd8d-29af0095460a@redhat.com> Subject: Re: [Qemu-devel] [PATCH v11 6/6] qom: support arbitrary non-scalar properties with -object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Markus Armbruster , Max Reitz , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= On Mon, Sep 12, 2016 at 01:20:25PM -0500, Eric Blake wrote: > On 09/05/2016 10:16 AM, Daniel P. Berrange wrote: > > The current -object command line syntax only allows for > > creation of objects with scalar properties, or a list > > with a fixed scalar element type. Objects which have > > properties that are represented as structs in the QAPI > > schema cannot be created using -object. > > > > > The previously added qdict_crumple() method is able to > > take a qdict containing a flat set of properties and > > turn that into a arbitrarily nested set of dicts and > > lists. By combining qemu_opts_to_qdict and qdict_crumple() > > together, we can turn the opt string into a data structure > > that is practically identical to that passed over QMP > > when defining an object. The only difference is that all > > the scalar values are represented as strings, rather than > > strings, ints and bools. This is sufficient to let us > > replace the OptsVisitor with the QMPInputVisitor for > > QObjectInputVisitor > > > use with -object. > > > > Thus -object can now support non-scalar properties, > > for example the QMP object > > > > { > > "execute": "object-add", > > "arguments": { > > "qom-type": "demo", > > "id": "demo0", > > "parameters": { > > "foo": [ > > { "bar": "one", "wizz": "1" }, > > { "bar": "two", "wizz": "2" } > > ] > > } > > } > > } > > > > Would be creatable via the CLI now using > > > > $QEMU \ > > -object demo,id=demo0,\ > > foo.0.bar=one,foo.0.wizz=1,\ > > foo.1.bar=two,foo.1.wizz=2 > > > > Notice that this syntax is intentionally compatible > > with that currently used by block drivers. > > > > This is also wired up to work for the 'object_add' command > > in the HMP monitor with the same syntax. > > > > (hmp) object_add demo,id=demo0,\ > > foo.0.bar=one,foo.0.wizz=1,\ > > foo.1.bar=two,foo.1.wizz=2 > > > > NB indentation should not be used with HMP commands, this > > is just for convenient formatting in this commit message. > > > > Signed-off-by: Daniel P. Berrange > > I still haven't looked closely at the intermediate patches in this > series, but I do like the end result, so I'll start with a review of > this one. > > > @@ -158,13 +167,21 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) > > { > > Visitor *v; > > QDict *pdict; > > + QObject *pobj; > > Object *obj = NULL; > > > > - v = opts_visitor_new(opts); > > pdict = qemu_opts_to_qdict(opts, NULL); > > > > - obj = user_creatable_add(pdict, v, errp); > > + pobj = qdict_crumple(pdict, true, errp); > > + if (!pobj) { > > + goto cleanup; > > + } > > + v = qobject_string_input_visitor_new(pobj); > > + > > + obj = user_creatable_add((QDict *)pobj, v, errp); > > This cast looks fishy; I think you want qobject_to_qdict(pobj) for > absolute safety (if some future change causes QDict to not contain > QObject at offset 0). Besides, qdict_crumple() can return a QList > instead of a QDict in some cases, so asserting that qobject_to_qdict() > did not return NULL may be worthwhile to prove that this particular > crumple never gives us something unexpected. > > > +static void test_dummy_createopts(void) > > +{ > > + const char *optstr = "qemu-dummy,id=dummy0,bv=yes,av=alligator,sv=hiss," > > + "person.name=fred,person.age=52,sizes.0=12,sizes.1=65,sizes.2=8139," > > + "addrs.0.ip=127.0.0.1,addrs.0.prefix=24,addrs.0.ipv6only=yes," > > + "addrs.1.ip=0.0.0.0,addrs.1.prefix=16,addrs.1.ipv6only=no"; > > + QemuOpts *opts; > > Actually, what happens if I do an optstr of "qemu-dummy,1=foo,2=bar"? > Is that rejected (because '1' and '2' are not valid key names) or does > it try to create a 2-element list instead of the usual dict? That is, > if a list is something the user can trigger at the CLI, then we need > better error handling than just an assertion that we actually get a > QDict above. With that example you give, qdict_crumple() will raise an error because you have a mixture of list and non-list keys at the top level. IOW, the crumple method is enforcing that you must only have a dict at the top. I'll add a test for this to validate the behaviour too. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|