Paolo Bonzini <pbonz...@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.
