On 10/21/2016 11:55 AM, Markus Armbruster wrote:

> 
> On first glance, this patch makes the boolean values recognized on the
> command line consistent regardless of which part of the code is used to
> recognize them, by extending the QemuOpts parser to accept the option
> visitor's additional convenience values.
> 
> Once we've done that in a release, we can't go back.  No problem if it
> actually achieved consistency.  But it doesn't: there are other parsers
> that recognize only "on" and "off".  A quick grep finds
> select_display(), bdrv_open_inherit(), netfilter_set_status().

Would it be wrong to convert these to use the same function, to get us
to the point where they are consistent instead of self-limited?  Yes, it
means that newer qemu will accept spellings that older doesn't, and that
we can't later prune the set of spellings back down, but trying to
remain backwards-compatible to every different set of inputs on a
command-by-command basis is harder than just making all commands accept
all spellings.

> 
> bdrv_open_inherit() is particularly instructive: -drive gets parsed by
> QemuOpts (evidence: your patch makes read-only=y work), but by the time
> the options arrive here, they're a QDict.  Curiously,
> bdrv_open_inherit() expects the value of key "read-only" to be a
> *string*, and it checks for "on".  Fortunately, something on the way to
> bdrv_open_inherit() replaces the "y" by "on", so this works.  My point
> is: this patch is trickier than it looks on first glance.
> 
> There's also HMP, which continues to accept only "on" and "off".

Particularly for HMP, where we DON'T have backwards-compatibility
constraints, I'd argue that ease-of-use is the driving factor and that
HMP should allow ALL spellings of boolean arguments.

> 
> We might want to treat the option visitor's additional boolean values
> like its other syntax extensions: keep for compatibility, but confine
> to existing uses.

As in - new interfaces ONLY get to pass "true" or "false", and only
existing interfaces get to also pass "y" or "on"?

> 
> I think we should decide that when we merge the rest of your option
> visitor replacement work, hopefully in 2.9.
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org