Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/24/2017 01:58 AM, Markus Armbruster wrote:
>
>>> I hate the 'no$key' sugar, and would love to get rid of it.  It makes
>>> things ambiguous (thus confusing) for precious little gain: 'novocaine'
>>> can mean 'novocaine=on' or 'vocaine=off'.  QemuOpts picks the latter,
>>> even when a QemuOpt named 'novocaine' has been declared.
>>>
>>> keyval_parse() is meant to behave like QemuOpts, so I'm reproducing this
>>> bit of bad sugar, reluctantly.
>> 
>> Implied value alternatives / variations:
>> 
>> (1) Don't implement the "no" sugar
>> 
>>     I think we'd have to deprecate it in QemuOpts now, to give users
>>     some time to adapt before keyval_parse() replaces QemuOpts.
>> 
>
>> 
>> If we're happy with bad sugar like "can't abbreviate novocaine=on to
>> novocaine" (confusing), "filename without a value gets you the file
>> 'on'", we reimplement the bad sugar and move on.
>> 
>> If not, we have the choice between less magic or more magic.
>> 
>> Less magic: (1) and/or (2).
>> 
>> More magic: (3) or something like it.
>> 
>> If we don't want to decide right now, we can do
>> 
>> (4) Don't implement any implied value sugar for now
>> 
>>     You have to spell out =on and =off.  Big fat comment to remind us
>>     about the QemuOpts incompatibility.
>> 
>> I think this would be acceptable for -blockdev in 2.9.
>> 
>> Opinions?
>
> Do any existing blockdev sub-parameters begin with 'no'?  Ouch:
> BlockdevOptions has '*cache':'BlockdevCacheOptions' which in turn has
> '*no-flush':'bool'.

I hate negative flags.

>                      Even worse, I think you could abuse 'nono-flush' to
> enable flushing - my head hurts.

"no-flush" gets desugared to "-flush=off", which doesn't work.

"nono-flush" gets desugared to "no-flush=off", which works.

> And of course, there's the obvious BlockDevOptions '*node-name':'str'.
> No one in their right mind would expect 'node-name' without parameters
> to result in 'de-name=no'.

This is something I'd be even willing to break for existing options.

> Also, I found that at least libvirt has existing command line usage
> -numa node,nodeid=0, where qemu_numa_opts uses .implied_opt_name="type",
> .desc = {{0}} to be visited by OptsVisitor.  Because of the implied opt,
> we get the desired "type=node" rather than the accidental "de=no", but
> the magic of implied keys can certainly make it weird to think about.

Correct.

The value of an implied key is not subject to key desugaring.

Without an implied key, the "node" would desugar to "de=off".

> How about:
>
> for 2.9: -blockdev has no magic at all (you HAVE to spell 'foo=off'
> rather than 'foo' for any boolean parameters that you want disabled in
> -blockdev);

You mean "rather than 'nofoo'".

"No magic at all" implies you also have to spell "foo=on" rather than
"foo".

I believe we need to have both or none in 2.9.  If we have only the "on"
sugar, "no-flush" works, and we can't add the "off" sugar without
breaking it.

> -blockdev); and in QemuOpts, we issue deprecation warnings but keep
> existing behavior any time someone uses 'noFOO' that we turn into
> 'FOO=no'.  Then, for 2.10, we can decide to remove the deprecation
> warnings if they pointed out real (ab)use of the sugar in the wild, or
> (hopefully) to kill the sugar entirely (as part of converting QemuOpts
> over to keyval_parse).
>
> In short, getting rid of the 'no' prefix magic, after suitable
> deprecation warnings, seems like a good plan to me; and having -blockdev
> be slightly tighter than the rest of command line options for 2.9 only
> is no real loss (no one uses -blockdev yet, so they can be written to
> avoid the use of 'no' magic to begin with).

Okay, I'll post QemuOpts patch to deprecate noKEY sugar.  Let's see
whether anyone screams.