Re: [PATCH v4 1/7] keyval: Fix and clarify grammar

[Markus Armbruster <armbru@redhat.com>]

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!