Re: [Qemu-devel] [PATCH v11 5/6] qapi: add a QmpInputVisitor that does string conversion

[Daniel P. Berrange]

On Tue, Sep 13, 2016 at 03:32:33PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> writes:
> > When using QemuOpts w/ qdict_crumple + QmpInputVisitor, you would
> > do list of scalars in different manner  'foo.1=3,foo.2=5,foo.3=533'
> > since this syntax is extendable to deal with arbitrary nesting of
> > dicts + lists, where as the OptsVisitor syntax cannot be extended
> > in a back-compatible manner.
> >
> > This series converts -object to use QmpInputVisitor and this is
> > safe todo right now, since no currently created QOM object has
> > a property which is a list of scalars and this the changed syntax
> > is not going to affect any existing usage.
> 
> Peeking at the patch... aha!  Instead of having the options visitor
> visit the QemuOpts, you convert the QemuOpts to a QDict, which you then
> visit with your new visitor.  Less efficient, because you have to build
> and destroy the intermediate QDict.  Not an issue when processing
> configuration or even monitor commands, of course.
> 
> I guess you convert to QDict so that the work of going from a
> QemuOpts-style string using -drive conventions to a visit splits into
> manageable chunks more easily, preferably into chunks that already
> exist: parse string into QemuOpts, convert to QDict, crumple, visit,
> destroy QDict.

FWIW, I did originally try to modify the QemuOpts visitor directly
to support visiting nested data structure, but I basically ended
up re-inventing alot of code from qdict and indeed having to use
a  qdict internally to QemuOpts to store intermediate bits. At
that point I realized it was clearly silly to try to shoe horn
it into QemuOpts visitor directly, since I was basically creating
qdict_crumple() indirectly and then essentially making QemuOpts
visitor have the same logic as QmpInputVisitor.

> Still, I take the conversion as a signal that our data structures are
> wrong.  Wild idea: should QemuOpts use a QDict rather than a QTAILQ of
> QemuOpt to store options?

If QemuOpts used a QDict internally, then you avoid the first
qemu_opts_to_qdict() call before qdict_crumple(), but everything
else basically stays the same. So a minor improvement, but nothing
ground-shaking.

> The pipeline then becomes parse string into QemuOpts, clone its QDict,
> crumple, visit, destroy QDict.  Next step would be crumpling in place,
> i.e. parse string into nested QemuOpts, get its QDict, visit.  Mind, I'm
> not demanding you to do that now, I'm just trying to figure out whether
> this series is shoveling into or out of the QemuOpts hole :)

Yes, I guess the interesting win would be if qemu_opts_do_parse()
could directly populate a qdict in crumpled format from the start,
thus avoiding the late qdict_crumple call altogether.

> > Other args would have to be considered on a case-by-case basis
> > to see if they rely on the magic list of scalars syntax used
> > by OptsVisitor. Probably only a few of them need this.
> 
> Obvious maintainer question: what would it take to get rid of the
> options visitor entirely?  Because this maintainer looks on additions to
> his fiefdom much more kindly when they're balanced by subtractions.
> 
> I guess all the uses that don't rely on the "list of scalars" magic are
> easy prey: replace by conversion to intermediate QDict + your new
> visitor.
> 
> What about the ones that do rely on it?  Which ones do?  Any ideas on
> how to change them so that they don't require a complete options
> visitor?

I guess you could have some code that re-processes the stored QemuOpt
in-place, changing repeated keys such as 'foo=400,foo=342' into the
preferred format 'foo.1=400,foo.2=342', at which point you can use
the qdict_crumple facility directly.

That's probably not even very hard.

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 :|