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 <berra...@redhat.com>
>
> 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 :|
