Am 07.10.2020 um 19:29 hat Eric Blake geschrieben: > On 10/7/20 11:49 AM, Kevin Wolf wrote: > > This adds a special meaning for 'help' and '?' as options to the keyval > > parser. Instead of being an error (because of a missing value) or a > > value for an implied key, they now request help, which is a new boolean > > ouput of the parser in addition to the QDict. > > output > > > > > A new parameter 'p_help' is added to keyval_parse() that contains on > > return whether help was requested. If NULL is passed, requesting help > > results in an error and all other cases work like before. > > > > Turning previous error cases into help is a compatible extension. The > > behaviour potentially changes for implied keys: They could previously > > get 'help' as their value, which is now interpreted as requesting help. > > > > This is not a problem in practice because 'help' and '?' are not a valid > > values for the implied key of any option parsed with keyval_parse(): > > > > * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, > > "help" and "?" are not among its values > > > > * display: union DisplayOptions, implied key "type" is enum > > DisplayType, "help" and "?" are not among its values > > > > * blockdev: union BlockdevOptions, implied key "driver is enum > > BlockdevDriver, "help" and "?" are not among its values > > > > * export: union BlockExport, implied key "type" is enum BlockExportType, > > "help" and "?" are not among its values > > > > * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, > > missing space > > > "help" and "?" are not among its values > > > > * nbd-server: struct NbdServerOptions, no implied key. > > Including the audit is nice (and thanks to Markus for helping derive the > list). > > > > > Signed-off-by: Kevin Wolf <[email protected]> > > --- > > > +++ b/util/keyval.c > > @@ -14,7 +14,7 @@ > > * KEY=VALUE,... syntax: > > * > > * key-vals = [ key-val { ',' key-val } [ ',' ] ] > > - * key-val = key '=' val > > + * key-val = 'help' | key '=' val > > Maybe: = 'help' | '?' | key = '=' val > > > * key = key-fragment { '.' key-fragment } > > * key-fragment = / [^=,.]* / > > * val = { / [^,]* / | ',,' } > > @@ -73,10 +73,14 @@ > > * > > * Additional syntax for use with an implied key: > > * > > - * key-vals-ik = val-no-key [ ',' key-vals ] > > + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] > > and again for '?' here.
Ah, true, I should mention '?', too.
> Actually, this should probably be:
>
> key-vals-ik = 'help' [ ',' key-vals ]
> | '?' [ ',' key-vals ]
> | val-no-key [ ',' key-vals ]
I noticed that the grammar was wrong even before my patch (because
making use of the implied key is optional), but the right fix wasn't
obvious to me, so I decided to just leave that part as it is.
Even your version is wrong because it's valid to a value with implied
key and help at the same time.
Thinking a bit more about it, I feel it should simply be something like:
key-vals-ik = val-no-key [ ',' key-vals ] | key-vals
And then key-vals automatically covers the help case.
> > * val-no-key = / [^=,]* /
>
> This is now slightly inaccurate, but I don't know how to concisely
> express the regex [^=,]* but not '?' or 'help', and the prose covers the
> ambiguity.
>
>
> > @@ -410,6 +442,14 @@ QDict *keyval_parse(const char *params, const char
> > *implied_key,
> > implied_key = NULL;
> > }
> >
> > + if (p_help) {
> > + *p_help = help;
> > + } else if (help) {
> > + error_setg(errp, "Help is not available for this option");
> > + qobject_unref(qdict);
> > + return NULL;
> > + }
>
> This part is a definite improvement over v2.
>
> I'm assuming Markus can help touch up the comments and spelling errors, so:
>
> Reviewed-by: Eric Blake <[email protected]>
I assumed that as a qsd series this would go through my own tree anyway,
so if all of you agree that you don't want to see the corrected version
on the list, I can fix them while applying the series.
Kevin
signature.asc
Description: PGP signature
