Eric Blake <ebl...@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 <arm...@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 <ebl...@redhat.com>
Thanks!
