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

[Markus Armbruster]

Fried brain today, I have to go through this real slow.  I apologize in
advance for being denser and more error-prone than usual.

Kevin Wolf <kwolf@redhat.com> writes:

> This adds a new parameter 'help' to keyval_parse() that enables parsing
> of help options. If NULL is passed, the function behaves the same as
> before. But if a bool pointer is given, it contains the information
> whether an option "help" without value was given (which would otherwise
> either result in an error or be interpreted as the value for an implied
> key).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

You change the parser, but neglected to update the grammar in the big
file comment.

I made the mistake of attempting to review the parser change without
fixing up the grammar first, and got lost in the weeds.  In part because
today is not my day, but also because doing parsers without a grammar
never works out well for me.

Grammar from the big file comment:

 * KEY=VALUE,... syntax:
 *
 *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
 *   key-val      = key '=' val
 *   key          = key-fragment { '.' key-fragment }
 *   key-fragment = / [^=,.]* /
 *   val          = { / [^,]* / | ',,' }

and

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

According to the commit message, we want to recognize "help" in place of
key-val and val-no-key, but only for callers that opt in.

This is more complex than recognizing it in place of just key-vals.  The
commit message doesn't say why the extra complexity is wanted.  Peeking
ahead in the series, I can see it's for supporting

    -object TYPE,help

in addition to

    -object help

I see.

Making help support opt-in complicates things.  Is there a genuine use
for not supporting help?  Or is just to keep the users that don't
support help yet (but should) working without change?  Mind, I'm not
asking you to make them work, I'm only asking whether you can think of a
genuine case where help should not work.

What are the existing users that don't support help?  Let's see... many
in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().
That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and
for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.

Attempting to get help for them fails like this:

    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help
    qemu-storage-daemon: Invalid parameter 'help'
    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help
    qemu-storage-daemon: Expected '=' after parameter 'help'

I believe making them fail like

    qemu-storage-daemon: no help available

would actually be an improvement.

If we get rid of the case "help is not supported", the grammar update
isn't too bad, something like

 *   key-val      = 'help' | key '=' val

and

 *   key-vals-ik  = 'help' | val-no-key [ ',' key-vals ]

Done for today, hope my brain unfries itself overnight.

[...]