The grammar has a few issues:

* key-fragment = / [^=,.]* /

  Prose restricts key fragments: they "must be valid QAPI names or
  consist only of decimal digits".  Technically, '' consists only of
  decimal digits.  The code rejects that.  Fix the grammar.

* val          = { / [^,]* / | ',,' }

  Use + instead of *.  Accepts the same language.

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

  The code rejects an empty value.  Fix the grammar.

* Section "Additional syntax for use with an implied key" is
  confusing.  Rewrite it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/keyval.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/util/keyval.c b/util/keyval.c
index 13def4af54..82d8497c71 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -16,8 +16,8 @@
  *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
  *   key-val      = key '=' val
  *   key          = key-fragment { '.' key-fragment }
- *   key-fragment = / [^=,.]* /
- *   val          = { / [^,]* / | ',,' }
+ *   key-fragment = / [^=,.]+ /
+ *   val          = { / [^,]+ / | ',,' }
  *
  * Semantics defined by reduction to JSON:
  *
@@ -71,12 +71,16 @@
  * Awkward.  Note that we carefully restrict alternate types to avoid
  * similar ambiguity.
  *
- * Additional syntax for use with an implied key:
+ * Alternative syntax for use with an implied key:
  *
- *   key-vals-ik  = val-no-key [ ',' key-vals ]
- *   val-no-key   = / [^=,]* /
+ *   key-vals     = [ key-val-1st { ',' key-val } [ ',' ] ]
+ *   key-val-1st  = val-no-key | key-val
+ *   val-no-key   = / [^=,]+ /
  *
- * where no-key is syntactic sugar for implied-key=val-no-key.
+ * 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 ','.
  */
 
 #include "qemu/osdep.h"
-- 
2.26.2

On 10/11/20 2:34 AM, Markus Armbruster wrote:
> The grammar has a few issues:
> 
> * key-fragment = / [^=,.]* /
> 
>    Prose restricts key fragments: they "must be valid QAPI names or
>    consist only of decimal digits".  Technically, '' consists only of
>    decimal digits.  The code rejects that.  Fix the grammar.
> 
> * val          = { / [^,]* / | ',,' }
> 
>    Use + instead of *.  Accepts the same language.
> 
> * val-no-key   = / [^=,]* /
> 
>    The code rejects an empty value.  Fix the grammar.
> 
> * Section "Additional syntax for use with an implied key" is
>    confusing.  Rewrite it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/keyval.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/util/keyval.c b/util/keyval.c
> index 13def4af54..82d8497c71 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -16,8 +16,8 @@
>    *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
>    *   key-val      = key '=' val
>    *   key          = key-fragment { '.' key-fragment }
> - *   key-fragment = / [^=,.]* /
> - *   val          = { / [^,]* / | ',,' }
> + *   key-fragment = / [^=,.]+ /

This requires a non-empty string.  Good (we don't allow an empty key).

> + *   val          = { / [^,]+ / | ',,' }

I agree that this has no real change.  Previously, you allowed zero or 
more repetitions of a regex that could produce zero characters; now, 
each outer repetition must make progress.

>    *
>    * Semantics defined by reduction to JSON:
>    *
> @@ -71,12 +71,16 @@
>    * Awkward.  Note that we carefully restrict alternate types to avoid
>    * similar ambiguity.
>    *
> - * Additional syntax for use with an implied key:
> + * Alternative syntax for use with an implied key:
>    *
> - *   key-vals-ik  = val-no-key [ ',' key-vals ]
> - *   val-no-key   = / [^=,]* /
> + *   key-vals     = [ key-val-1st { ',' key-val } [ ',' ] ]
> + *   key-val-1st  = val-no-key | key-val
> + *   val-no-key   = / [^=,]+ /
>    *
> - * where no-key is syntactic sugar for implied-key=val-no-key.
> + * 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 ','.

Nor can you use the sugared form when the value is intended to be empty 
(although this may be academic, as your other patches enumerate the list 
of clients, and none of them seem to allow an empty value even when 
desugared).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 10/11/20 2:34 AM, Markus Armbruster wrote:
>> The grammar has a few issues:
>> * key-fragment = / [^=,.]* /
>>    Prose restricts key fragments: they "must be valid QAPI names or
>>    consist only of decimal digits".  Technically, '' consists only of
>>    decimal digits.  The code rejects that.  Fix the grammar.
>> * val          = { / [^,]* / | ',,' }
>>    Use + instead of *.  Accepts the same language.
>> * val-no-key   = / [^=,]* /
>>    The code rejects an empty value.  Fix the grammar.
>> * Section "Additional syntax for use with an implied key" is
>>    confusing.  Rewrite it.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   util/keyval.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>> diff --git a/util/keyval.c b/util/keyval.c
>> index 13def4af54..82d8497c71 100644
>> --- a/util/keyval.c
>> +++ b/util/keyval.c
>> @@ -16,8 +16,8 @@
>>    *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
>>    *   key-val      = key '=' val
>>    *   key          = key-fragment { '.' key-fragment }
>> - *   key-fragment = / [^=,.]* /
>> - *   val          = { / [^,]* / | ',,' }
>> + *   key-fragment = / [^=,.]+ /
>
> This requires a non-empty string.  Good (we don't allow an empty key).
>
>> + *   val          = { / [^,]+ / | ',,' }
>
> I agree that this has no real change.  Previously, you allowed zero or
> more repetitions of a regex that could produce zero characters; now, 
> each outer repetition must make progress.
>
>>    *
>>    * Semantics defined by reduction to JSON:
>>    *
>> @@ -71,12 +71,16 @@
>>    * Awkward.  Note that we carefully restrict alternate types to avoid
>>    * similar ambiguity.
>>    *
>> - * Additional syntax for use with an implied key:
>> + * Alternative syntax for use with an implied key:
>>    *
>> - *   key-vals-ik  = val-no-key [ ',' key-vals ]
>> - *   val-no-key   = / [^=,]* /
>> + *   key-vals     = [ key-val-1st { ',' key-val } [ ',' ] ]
>> + *   key-val-1st  = val-no-key | key-val
>> + *   val-no-key   = / [^=,]+ /
>>    *
>> - * where no-key is syntactic sugar for implied-key=val-no-key.
>> + * 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 ','.
>
> Nor can you use the sugared form when the value is intended to be
> empty

True.  Spelling it out wouldn't hurt.  Takes a follow-up patch; this one
is already in master.

>       (although this may be academic, as your other patches enumerate
> the list of clients, and none of them seem to allow an empty value
> even when desugared).
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!