Re: [Qemu-devel] [PATCH v14 01/21] option: make parse_option_bool/number non-static

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

I have two concerns:

1. Accepting alternate spellings in just a few places is slightly bad,
accepting them is most, but not all places is worse.  Converting all the
parsers as you suggest would take care of this one.

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

2. I'm scared some user of QemuOpts declares QEMU_OPT_BOOL, but then
checks the *string*.  Yes, QemuOpts lets you do that.  Making the parser
accept more strings would break such code.  Need a careful code review
to put this concern to rest.

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

Personally, I find alternate spellings more confusing than helpful, but
I agree that once we adopt them for the command line (consistently, not
just in a few places), we should also adopt them for HMP.

>> 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"?

Very few places accept anything but "on" and "off", namely the ones that
use the options or the string visitor.  Dan's work kills of the former,
and hopefully permits us killing off the latter.  The alternate
spellings will then be confined in one place, guarded by a compatibility
flag you're not supposed to set in new code.  Looks workable to me.

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

I'm not rejecting this patch, I'm merely punting the ABI-affecting parts
of this series to 2.9.