Markus Armbruster <[email protected]> writes:
> Fabiano Rosas <[email protected]> writes:
>
>> Markus Armbruster <[email protected]> writes:
>>
>>> Fabiano Rosas <[email protected]> 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 <[email protected]>
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.
>>>
>>> [...]
