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

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH v3 1/4] keyval: Parse help options
Date: Thu, 08 Oct 2020 17:25:57 +0200	[thread overview]
Message-ID: <87wo00viwq.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201008084531.GC4672@linux.fritz.box> (Kevin Wolf's message of "Thu, 8 Oct 2020 10:45:31 +0200")

Kevin Wolf <kwolf@redhat.com> writes:

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

Let's use a non-terminal

    help = 'help' | '?'
    key-val = key '=' val | help

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

The grammar leaves a small, but important unsaid, or rather said only in
the accompanying prose.

 * Additional syntax for use with an implied key:
 *
 *   key-vals-ik  = val-no-key [ ',' key-vals ]
 *   val-no-key   = / [^=,]* /

tries to express that with an implied key, the grammar changes from

     S = key-vals

to

     S = key-vals | key-vals-ik

See, "additional syntax".  Felt clear enough to me when I wrote it.  It
is not.

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

Unfortunately, this simple grammar is ambigious: "help" can be parsed
both as

    "help" -> help -> key-val -> key-vals -> key-vals-ik

and

    "help" -> val-no-key -> key-vals-ik

Ham-fisted fix:

    val-no-key = / [^-,]* / - 'help'

I'm not a fan of the exception operator.  Better ideas?

I'll have a closer look at the actual patch next.

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

The series spans "Command line option argument parsing" (this patch),
"QOM" (next two), and qsd (final one).  I'm fine with you taking it
through your tree.

Just noticed: you neglected to cc: the QOM maintainers.  Recommend to
give them a heads-up.