Re: [PATCH v3 1/4] keyval: Parse help options

From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v3 1/4] keyval: Parse help options
Date: Thu, 8 Oct 2020 10:45:31 +0200	[thread overview]
Message-ID: <20201008084531.GC4672@linux.fritz.box> (raw)
In-Reply-To: <609ce08c-35d5-ca85-ac15-bdbc56c28465@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 4288 bytes --]

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 <kwolf@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 <eblake@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]