Re: [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()

[Eric Blake]

On 02/28/2017 12:03 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 02/28/2017 09:48 AM, Kevin Wolf wrote:
>>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>>>> qemu_opts_parse(), except:
>>>>
>>
>>>> +
>>>> +/*
>>>> + * KEY=VALUE,... syntax:
>>>> + *
>>>> + *   key-vals     = [ key-val { ',' key-vals } ]
>>
>> Just refreshing my memory: in this grammar, [] means optional (0 or 1),
>> and {} means repeating (0 or more).
> 
> Yes.
> 
>> That means an empty string satisfies key-vals (as in "-option ''"),
>> intentional?
> 
> Yes.  Creates an empty root object.  If we don't want that, or don't
> want it now, I can make this case an error.

Making it an error now, with the ability to relax it later, would be in
line with the fact that dotted notation cannot create an empty
sub-element.  Supporting it now means we're stuck with it even if we
decide the empty string could have usefully meant something else
(although what else is useful besides an empty root object is beyond me).

> 
>> I don't see how this permits a trailing comma, but isn't that one of
>> your goals to allow "-option key=val," the same as "-option key=val"?
> 
> Mistake.  Possible fix:
> 
>           key-vals     = [ key-val { ',' key-val } [ ',' ]

Unbalanced [].  I think you meant:

key-vals = [ key-val { ',' key-val } [ ',' ] ]

which properly rejects "-option ," while allowing "-option key=val,".


> 
>>>> + *   key-val      = key '=' val
>>>> + *   key          = key-fragment { '.' key-fragment }
>>
>> Ambiguous.
> 
> I'm dense.  Can you give me an example string with two derivations?

Sorry, poor editing on my part. (I wrote that before I figured out that
{} meant 0 or more, then forgot to clean it up).  Looks like this one is
well-formed after all.

> 
>>>> + *   key-fragment = / [^=,.]* /
>>
>> Do you want + instead of * in the regex, so as to require a non-empty
>> string for key-fragment?  After all, you want to reject "-option a..b=val".
> 
> I like to keep syntactic and semantic analysis conceptually separate.
> keyval_parse() looks for the next '.' to extract a key-fragment
> (syntactic analysis).  It then rejects key-fragments it doesn't like
> (semantic analysis).  Right now, it only dislikes lengths outside
> [1,127].  Later on, it'll additionally dislike key-fragments that are
> neither valid QAPI names nor digit strings.
> 
> Perhaps my comment could explain this better.

Yes, a comment would help (then keeping the grammar accepting a 0-length
string doesn't hurt, because the semantic analysis kicks in).


>>>> + *
>>>> + * Semantics defined by reduction to JSON:
>>>> + *
>>>> + *   key-vals defines a tree of objects rooted at R
>>>> + *   where for each key-val = key-fragment . ... = val in key-vals
>>>> + *       R op key-fragment op ... = val'
>>>> + *       where (left-associative) op is member reference L.key-fragment
>>>
>>> Maybe it's just me, but I can't say that I fully understand what these
>>> last two lines are supposed to tell me.
>>
>> I think it's trying to portray dictionary member lookup semantics (each
>> key-fragment represents another member lookup one dictionary deeper,
>> before reaching the final lookup to the scalar value) - but yeah, it was
>> a confusing read to me as well.
> 
> We're in a bit of a time squeeze right now.  I'd like to clarify the
> comment on top.

That's fair.  A poor comment isn't code, so fixing it in soft freeze is
fine.  Maybe a FIXME is still appropriate, if we don't want to forget
it, but I trust your judgment on this one.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature