Re: [PATCH 04/25] keyval: accept escaped commas in implied option

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

[...]
> diff --git a/util/keyval.c b/util/keyval.c
> index be34928813..eb9b9c55ec 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -16,8 +16,8 @@
>   *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
>   *   key-val      = key '=' val | help
>   *   key          = key-fragment { '.' key-fragment }
> - *   key-fragment = / [^=,.]+ /
> - *   val          = { / [^,]+ / | ',,' }
> + *   key-fragment = { / [^=,.] / | ',,' }
> + *   val          = { / [^,] / | ',,' }
>   *   help         = 'help' | '?'
>   *
>   * Semantics defined by reduction to JSON:
> @@ -78,13 +78,13 @@
>   * Alternative syntax for use with an implied key:
>   *
>   *   key-vals     = [ key-val-1st { ',' key-val } [ ',' ] ]
> - *   key-val-1st  = val-no-key | key-val
> - *   val-no-key   = / [^=,]+ / - help
> + *   key-val-1st  = (val-no-key - help) | key-val
> + *   val-no-key   = { / [^=,] / | ',,' }

I finally remembered why I made val-no-key non-empty: to avoid amiguity.

Before the patch, "" can only be parsed as empty key-vals.  Results in
an empty QDict.

Afterwards, the grammar is ambiguous: "" can also be parsed as empty
val-no-key, reduced via key-val-1st to non-empty key-vals.  Results in a
QDict with one entry mapping the implied key to "".

I'm a bit concerned I similarly forgot something that made me avoid ',,'
escapes in val-no-key.

Even if we brushed off the ambiguous grammar issue (and we should not!),
desugaring "" into "implied=" feels unwise, and ",k=v" into
"implied=,k=v" only slightly less so.

Let's keep val-no-key non-empty.

Ripple effect...  I made val-no-key match key (almost):

    val-no-key   = / [^=,]+ /

    key          = key-fragment { '.' key-fragment }
    key-fragment = / [^=,.]+ /

The only difference is val-no-key accepts consecutive '.'.

Commit 8bf12c4f75 "keyval: Parse help options" muddied the waters a bit
by adding '- help' to val-no-key.

Your commit moves it to key-val-1st (good).  It also permits empty
key-fragment, and thus consecutive '.' (good because it makes val-no-key
match key exactly, but also possibly confusing because key-fragment
can't actually be empty due to the "Key-fragments must be valid QAPI
names or consist only of decimal digits" condition).  Okay.

It also changed both val-no-key and key to accept empty.  We need to
keep *both* non-empty.

Your change to val is not wrong, but I prefer the old version, because
it's closer to how the code works.

>   *
>   * where val-no-key is syntactic sugar for implied-key=val-no-key.
>   *
> - * Note that you can't use the sugared form when the value contains
> - * '=' or ','.
> + * Note that you can't use the sugared form when the value is empty

You can with your grammar change, unless the code doesn't match the
grammar.  Which would be a bug.

> + * or contains '='.
>   */
[...]

I apologize for sitting on this patch for so long.  Something felt
wrong, but I couldn't put a finger on it.  Now I can at least for the
empty val-no-key part.