Re: [PATCH 1/2] keyval: accept escaped commas in implied option

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> [huge snip]
>
> On 27/11/20 09:38, Markus Armbruster wrote:
>> The suboptimal error message is due to the way I coded the parser, not
>> due to the grammar.
>
> Small digression: a different grammar influences how the parser is 
> written.  You coded the parser like this because you thought of implied 
> options as "key without ="; instead I thought of them as "value not 
> preceded by key=".

Which requires relaxing the key syntax, as you did.  Without that, we'd
need unbounded lookahead to decide how to reduce a comma.

>> 
>>   --nbd key=val,=,fob=
>> 
>>     master:       Invalid parameter ''
>>     your patch:   Expected parameter before '='
>> 
>>     Improvement, but which '='?  Possibly better:
>> 
>>                   Expected parameter before '=,fob='
>
> Yup, easy.
>
>>    --nbd .key=
>> 
>>      master:       Invalid parameter '..key'
>>      your patch:   same
>> 
>>      Better, I think:
>> 
>>                    Name expected before '..key'
>> 
>>    Odd: if I omit the '=', your patch's message often changes to
>> 
>>                    Expected '=' after parameter ...
>> 
>>    This means the parser reports a non-first syntax error.  Parsing
>>    smell, I'm afraid :)
>
> Nah, just lazy cut-and-paste of the existing error message.  I should 
> rename that error to something "No implicit parameter name for '.key'" 
> (again, different grammar -> different parser -> different error).  That 
> error message actually makes sense: "--object .key" would create an 
> object of type ".key" both without or with these changes.

However, --object a=b,.key would not, because the sugar is available
for the leftmost value only.

"No implicit parameter name" assumes the user intended .key as a value,
and forgot to write the key.  We could instead assume the user intended
.key as key, and messed it up (forgot a fragment, fat-fingered '.',
whatever).  The absence of '=' makes the value assumption more
plausible, but that's already lookahead.

Error messages based on guesses what the user has in mind can be quite
confusing when we guess wrong.  A strictly factual syntax error style
like "I expected FOO instead of BAR here" may not be great, but has a
relatively low risk of being confusing.

>> * Invalid key fragment
>> 
>>    --nbd key.1a.b=
>> 
>>      master:       Invalid parameter 'key.1a.b'
>>      your patch:   same
>> 
>>      Slightly better, I think:
>> 
>>                    'key.1a' is not a valid parameter name
>
> Or just "Invalid parameter '1a'".  I'm not going to do that in v2 
> though, since parameter parsing is not being

Sentence not being finished?

>> I believe there are two, maybe three reasons for this series:
>> 
>> 1. Support ',' in values with an implicit keys.
>> 
>> 2. Improve error reporting.
>> 
>> 3. Maybe nicer code.
>> 
>> 1. is a feature without a known use.
>
> Breaking news: there is actually a use.  I should have pointed out in 
> the commit message, but I didn't realize at the time, that this patch 
> fixes device-introspect-test once device_add is switched to keyval-based 
> parsing.  And why is that?  Because even though SUNW,fdtwo is not 
> user-creatable, you can still do "device_add SUNW,,fdtwo,help".  It even 
> works from the command line:
>
> $ qemu-system-sparc -device SUNW,,fdtwo,help
> SUNW,fdtwo options:
>    drive=<str>            - Node name or ID of a block device to use as 
> a backend
>    fallback=<FdcDriveType> - FDC drive type, 144/288/120/none/auto 
> (default: "144")
>    ...

Right.  I actually had that knowledge filed in my brain, but it failed
to bubble up.

It fixes device-introspect-test only because you also fixed the test to
escape comma (commit e27bd498769, in rc1).  Quoting myself: "Parameter
values with comma need special treatment before and after the patch."

> This invocation is useful (for some value of useful) to see which 
> properties you can pass with -global.  So there *is* a valid (for some 
> value of valid) use of escaped commas in implied options.  It can be 
> fixed with deprecation etc. but that would be a more complicated 
> endeavor than just adjusting keyval.

The question becomes whether CLI help syntax is subject to the
compatibility promise.

It's certainly not something we'd want programs to use.  We provide QMP
commands for the purpose.

For human users, the usability goodness of keeping

    -device SUNW,,fdtwo,help

working would in my opinion be dwarved several times over by renaming
the the offenders so that you don't need arcane knowledge "double the
comma" just to get help.  We should do that regardless of this patch.

>> 2. can be had with much less churn
>> (I'm ready to back that up with a patch).  Since I haven't looked at
>> PATCH 2, I'm reserving judgement on 3.
>
> FWIW I think this patch is already an improvement in code niceness, 
> though I accept that it's in the eye of the beholder.
>
> Paolo