Re: [PATCH 03/25] qemu-option: warn for short-form boolean options

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 19/01/21 16:56, Markus Armbruster wrote:
>>> +            if (!is_help && warn_on_flag) {
>>> +                warn_report("short-form boolean option '%s%s' deprecated", prefix, *name);
>>> +                error_printf("Please use %s=%s instead\n", *name, *value);
>>> +            }
>> 
>> If @warn_on_flag, we warn except for "help" and "?".  The exception
>> applies regardless of @help_wanted.  Shouldn't we except*only*
>> recognized help requests?
>
> Suggesting "help=yes" would be worse.

Would it?

get_opt_name_value() parses one parameter from params into *name and
*value.  if help_wanted && is_help, it additionally sets *help_wanted to
true.  is_help is true when the parameter is "help" or "?".

How could a parameter "help" be handled?

get_opt_name_value() will set

    *name = g_strdup("help");
    *value = g_strdup("on");

If help_wanted, additionally:

    *help_wanted = true;

Callers that pass non-null help_wanted can do whatever they want with
that.  The actual callers do honor the help request.  The deprecation
warning obviously needs to be suppressed for them.

Callers that pass null help_wanted will treat this just like any other
parameter.  Use of the boolean sugar is just as deprecated for this
parameter as it is for all the others.  Suppressing the deprecation
warning feels wrong.

The alternative is to *outlaw* parameters "help" and "?" in QemuOpts.
I'd be cool with that.

>>> -    opts = opts_parse(list, params, permit_abbrev, false,
>>> +    opts = opts_parse(list, params, permit_abbrev, false, true,
>>>                        opts_accepts_any(list) ? NULL : &help_wanted,
>>>                        &err);
>>>      if (!opts) {
>> 
>> This function now warns, except for "help" and "?".  The exception
>> applies even when we treat "help" and "?" as sugar for "help=on" and
>> "?=on" because opts_accepts_any().
>> 
>> It is the only spot that enables the warning.
>> 
>> Does all user input flow through qemu_opts_parse_noisily()?
>> 
>
> I was going to say yes, but -vnc (and worse, the QMP version of "change 
> vnc") is parsed by qemu_opts_parse() via ui/vnc.c (besides being used by 
> lots of tests).  -vnc has several boolean options, and though Libvirt 
> only uses "sasl" it does so in the short form.
>
> My solution would be to deprecate the QMP "change vnc" command, and 
> postpone switching -vnc to qemu_opts_parse_noisily to 6.2.

QMP command 'change' was deprecated long ago, in 2.5.0 (commit
24fb41330, in 2015).  This predated appendix "Deprecated features"
(which has since become docs/system/deprecated.rst), and remained
missing there until I corrected it in commit 6d570ca10 (v4.2.0).

> The main reason to warn for short-form boolean options, is to block them 
> for command line options that are switched to keyval[1].  Adding a 
> warning does not necessarily imply removing in two releases.

Understand.

> Paolo
>
> [1] This series already does that for -M, -accel and -object.  This 
> means that applying this series would change the command line 
> incompatibly without a two-release deprecation.  It's up for discussion 
> whether to do so, or delay the application of those patches to 6.2.  It 
> would be a pity to hold the dependent changes for effectively a year, 
> but it's not a big deal.

Concur.