Re: [PATCH v2 03/11] qemu-option: Remove short form options support

[Fabiano Rosas <farosas@suse.de>]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> Option parameters without a value (i.e. key vs. key=val) are
>>>> deprecated. The missing value is currently implied to be either "on"
>>>> or "off" depending on whether the parameter name starts with "no".
>>>
>>> Commit ccd3b3b811 (qemu-option: warn for short-form boolean options,
>>> 2020-11-09).  Six years of warnings should suffice.
>>>
>>>> Remove the implied behavior and start rejecting option parameters
>>>> without values by emitting the error message:
>>>>
>>>>  "Parameter 'foo' without a value."
>>>>
>>>> Two special cases remain:
>>>>
>>>> 1) The 'help' parameter is special and still supported. Keep a
>>>>    positive value attached to it ("on").
>>>>
>>>>    Note that currently there is no validation for the value (if any)
>>>>    attached to the help parameter. help, help=on, help=off, help=foo
>>>>    all result in the help text being emitted. This is not changed by
>>>>    this patch.
>>>
>>> Really?
>>>
>>>     $ qemu-system-x86_64 -chardev help=foo
>>>     qemu-system-x86_64: -chardev help=foo: Invalid parameter 'help'
>>>
>>
>> Ah, ok, it depends on whether the option accepts any parameter
>> (i.e. opts_accepts_any()). Here's -object failing to reject the bogus
>> value:
>>
>> $ qemu-system-x86_64 -object memory-backend-ram,id=m1,help=foo
>> memory-backend-ram options:
>>   dump=<bool>            - Set to 'off' to exclude from core dump
>>   host-nodes=<[uint16]>  - Binds memory to the list of NUMA host nodes
>>   merge=<bool>           - Mark memory as mergeable
>>   policy=<HostMemPolicy> - Set the NUMA policy
>>   prealloc-context=<link<thread-context>> - Context to use for creating CPU threads for preallocation
>>   prealloc-threads=<int> - Number of CPU threads to use for prealloc
>>   prealloc=<bool>        - Preallocate memory
>>   reserve=<bool>         - Reserve swap space (or huge pages) if applicable
>>   share=<bool>           - Mark the memory as private to QEMU or shared
>>   size=<size>            - Size of the memory region (ex: 500M)
>>   x-use-canonical-path-for-ramblock-id=<bool>
>
> Bizarre :)
>
>>>> 2) The empty value is allowed if the key is also empty. This can be
>>>>    achieved in the command line by adding commas.
>>>>    E.g.: share=on,, is parsed as:
>>>>      key:"share" value:"on" and
>>>>      key:"" value:"on"
>>>
>>> I don't think so:
>>>
>>>     $ qemu-system-x86_64 -object memory-backend-ram,id=mem0,share=on,,
>>>     qemu-system-x86_64: -object memory-backend-ram,id=mem0,share=on,,: Parameter 'share' expects 'on' or 'off'
>>>
>>
>> Bah, I wrote the example for the commit message but haven't tried
>> it. I'm trying to refer to the single leading comma scenario, we have a
>> test for it:
>>
>>     /* Except when it isn't */
>>     opts = qemu_opts_parse(&opts_list_03, ",", false, &error_abort);
>>     g_assert_cmpuint(opts_count(opts), ==, 1);
>>     g_assert_cmpstr(qemu_opt_get(opts, ""), ==, "on");
>
> The "except when it isn't" comment refers to the test right above:
>
>       /* Trailing comma is ignored */
>       opts = qemu_opts_parse(&opts_list_03, "x=y,", false, &error_abort);
>       g_assert_cmpuint(opts_count(opts), ==, 1);
>       g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, "y");
>
> This is about trailing comma.  Is the exception is only possible when
> the trailing comma is also the leading comma?
>
>> The following are parsed as valid and only reject the empty key further
>> down the line:
>>
>> $ qemu-system-x86_64 -drive ,
>> qemu-system-x86_64: -drive ,: warning: short-form boolean option '' deprecated
>> Please use =on instead
>> qemu-system-x86_64: -drive ,: Must specify either driver or file
>>
>> $ qemu-system-x86_64 -drive ,file=dummy
>> qemu-system-x86_64: -drive ,file=dummy: warning: short-form boolean option '' deprecated
>> Please use =on instead
>
> Looks like the answer is no.  The tests could be clearer there.  Might
> not matter after your series.
>
>> WARNING: Image format was not specified for 'dummy' and probing guessed raw.
>>          Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
>>          Specify the 'raw' format explicitly to remove the restrictions.
>> qemu-system-x86_64: -drive ,file=dummy: Block format 'raw' does not support the option ''
>
> The entire parser should be burned with fire.
>
> What are the remaining differences to the keyval.c parser after your
> series?  Can we deprecate them?
>

I ran a few of the qemu-opts tests with the keyval code and it differs
mostly in the handling of empty (non-implied) key and implied key +
empty value. I'll take a close look next week but I think we should
deprecate them anyway.

It looks easy to add a flag to keyval_parse_one() to turn its errors
into deprecation warnings ("warning: madness deprecated"). We could then
gradually make the conversion without having to wait for the deprecated
parts to be removed. The child is almost 10 years old.

  d454dbe0ee3 ("keyval: New keyval_parse()")
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   Tue Feb 28 22:26:49 2017 +0100

>>> Set a breakpoint on qapi_bool_parse() to see the actual value.  It's
>>> "on,".  In QemuOpts syntax, double comma is an escape, so you can put
>>> comma in values.
>>>
>>>> One effect of this change that might not be obvious is that parameter
>>>> combinations of the form:
>>>>
>>>>  id=mem0,share,help
>>>>
>>>> no longer produce the help output. The lack of value for the 'share'
>>>> parameter is reported with precedence. Users will need to either use a
>>>> valid value or omit the parameter entirely:
>>>>
>>>>  id=mem0,share=on,help
>>>>  id=mem0,help
>>>
>>> That's unfortunate.
>>>
>>> [...]