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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> The opts-visitor.c opts_type_bool() method has code for
> parsing a string to set a bool value, as does the
> qemu-option.c parse_option_bool() method, except it
> handles fewer cases.
>
> To enable consistency across the codebase, extend
> parse_option_bool() to handle "yes", "no", "y" and
> "n", and make it non-static. Convert the opts
> visitor to call this method directly.
>
> Also make parse_option_number() non-static to allow
> for similar reuse later.
>
> Reviewed-by: Kevin Wolf <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> Signed-off-by: Daniel P. Berrange <address@hidden>

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

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

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.

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