Re: [PATCH 05/25] keyval: simplify keyval_parse_one

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/01/21 14:48, Markus Armbruster wrote:
>>    --nbd .key=
>>      master:       Invalid parameter '..key'
>>      your patch:   same
>>      Likweise.
>>    If I omit the '=', your patch's message changes to
>>                    No implicit parameter name for value 'key..'
>>    I consider that worse than before, because it's talking about
>>    something outside the user's control (lack of an implict parameter
>>    name) where it should instead tell the user what needs fixing in the
>>    input.
>
> I think whether it's better or worse depends on the specific erroneous
> command line (think "--nbd /path/to/file.qcow2"), but I can certainly 
> change it.

Quoting myself: Error messages based on guesses what the user has in
mind can be quite confusing when we guess wrong.  A strictly factual
syntax error style like "I expected FOO instead of BAR here" may not be
great, but has a relatively low risk of being confusing.

>> Your patch also adds an "Expected parameter at end of string" error.
>> Can you tell me how to trigger it?
>
> It is meant for "--nbd ''" but it is effectively dead code due to the
> "while (*s)" in the caller.  Possibilities:
>
> 1) leave it in as dead code

I'd prefer not to.

> 2) replace it with an assert

Works for me.

> 3) change the caller to use a do...while in such a way that it
> triggers it (and be careful not to change the grammar).

Only if it's not too hard, and results in better error messages.

>> I believe your grammar is ambiguous.  Your code seems to pick the sane
>> alternative.  If I'm wrong, you need to enlighten me.  If I'm right, you
>> need to fix your grammar.
>
> Will do.  Can I change the EBNF to use "+" and "*" for simplicity and
> clarity?

I tried to stick to ISO 14977 as described in
<https://en.wikipedia.org/wiki/Extended_Backus%E2%80%93Naur_form>, less
its foolish ',' for concatenation.  I'm not at all enamored with it.
All I want is something that is widely understood, and preferably not
invented here.  Switch to <https://www.w3.org/TR/xml/#sec-notation>
perhaps?

[...]