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

[Paolo Bonzini]

[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=".

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

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

> 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")
  ...

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.

> 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