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 <kw...@redhat.com>
> > ---
>
> > +++ 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 <ebl...@redhat.com>
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
