"Daniel P. Berrange" <berra...@redhat.com> 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 <kw...@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> > Reviewed-by: Eric Blake <ebl...@redhat.com> > Reviewed-by: Markus Armbruster <arm...@redhat.com> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
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.
