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?
>> 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.
>>
>> [...]
