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:
>
>> 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>

>> 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 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
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 ''


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