Re: [Qemu-devel] [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optionally do string conversion

[Daniel P. Berrange]

On Thu, Jul 14, 2016 at 08:04:29AM -0600, Eric Blake wrote:
> On 07/14/2016 07:03 AM, Daniel P. Berrange wrote:
> 
> >>> I'm not really a huge fan of the approach there. IMHO the
> >>> input visitor should strictly operate in "all native types"
> >>> or "all string types" mode. The way you've done it here
> >>> means that users can actually mix & match strings vs native
> >>> types for each individual parameter :-(
> >>>
> >>> IMHO, I'd suggest you try to parse netdev_add args with
> >>> it in "all native types" mode, if that fails, then re-parse
> >>> in "all string types" mode.
> >>
> >> Sadly, this idea won't work. Without this series, the existing QMP
> >> command 'netdev_add' takes mixed integers and strings in a single call,
> >> by virtue of converting QObject into QemuOpts and back into QObject.
> > 
> > Ok, so lemme check I understand. The original QObject has properties
> > which are integers, but with existing code QMP allows them to be passed
> > as strings or ints, converts them to QemuOpts which makes them all
> > strings and converts them back to QObject leaving them all as strings.
> 
> Yes, the existing code (using 'gen':false) just passes an entire QObject
> to hand-written code; the hand-written code converted everything to
> QemuOpts (which flattens both integers and strings into options), then
> expanded QemuOpts back into QObject (strings-only).
> 
> > 
> > You've now got rid of the QObject -> QemuOpts conversion, so we're
> > not string-ifying everything, and so have to cope with arbitrary
> > mix directly.
> 
> Yes.
> 
> > 
> >> Forcing the parser to allow only integers or only strings would reject
> >> current uses of netdev_add, and we don't yet have a good idea whether
> >> any existing clients were depending on that behavior.
> >>
> >> Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse
> >> is by setting a single flag which controls which visitor constructor is
> >> used.  Your idea of parsing once with integer-only, then falling back to
> >> parse a second time with string-only, would complicate the generator to
> >> have to create two different visitors, rather than passing a single
> >> boolean parameter through to the single visitor constructor.
> > 
> > So I don't think a simple boolean flag is desirable, as we have 3 distinct
> > scenarios:
> > 
> >  1. Strongly typed only
> >  2. String conversion only
> >  3. Strongly typed, with string conversion fallback
> > 
> > My current patch provides 1 + 2, your alternative patch provides 1 + 3.
> > If we use option 3, in cases which only actually need option 2, then we
> > are potentially introducing more scenarios where arbitrarily mixed
> > types are accepted.  IOW, I think we must implement 1, 2 and 3 and avoid
> > using 3 except where absolutely needed.
> 
> 3 is a superset of 2, and your command-line conversion is the only case
> where we can achieve 2.  That is, code dealing with QMP can only choose
> between 1 and 3, based on whether the QAPI .json file used
> 'autocast':true for back-compat reasons (the only candidates are
> 'netdev_add' and 'device_add').  And code dealing with command line
> parsing can only choose 2 (QemuOpts is string-only), but parsing
> string-only via 2 is no different than the result achieved from parsing
> strongly-typed with string fallback via 3.
> 
> I still don't buy the fact that we need a string-only parser at the
> moment, but it would not be hard to change the 'bool autocast' into a
> tri-state enum, and then make the implementation specifically honor 1,
> 2, or 3 based on the enum value.

FYI, I just copied you on a patch that enables us to easily support
all 3 options.


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