[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
