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

[Daniel P. Berrange]

On Thu, Jul 14, 2016 at 06:43:16AM -0600, Eric Blake wrote:
> On 07/14/2016 02:04 AM, Daniel P. Berrange wrote:
> > On Wed, Jul 13, 2016 at 09:50:27PM -0600, Eric Blake wrote:
> >> Currently the QmpInputVisitor assumes that all scalar
> >> values are directly represented as their final types.
> >> ie it assumes an 'int' is using QInt, and a 'bool' is
> >> using QBool.
> >>
> >> This adds an alternative mode where a QString can also
> >> be parsed in place of the native type, by adding a parameter
> >> and updating all callers.  For now, only the testsuite sets
> >> the new autocast parameter.
> >>
> >> This makes it possible to use QmpInputVisitor with a QDict
> >> produced from QemuOpts, where everything is in string format.
> >> It also makes it possible for the next patch to support
> >> back-compat in legacy commands like 'netdev_add' that no
> >> longer use QemuOpts, but must parse the same set of QMP
> >> constructs that QemuOpts would historically allow.
> > 
> > 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.

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.

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

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