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

[Eric Blake]

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).

That means an empty string satisfies key-vals (as in "-option ''"),
intentional?

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"?

>> + *   key-val      = key '=' val
>> + *   key          = key-fragment { '.' key-fragment }

Ambiguous.

>> + *   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".

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

Here, * makes sense, since an empty value is permitted in '-option key=".

>> + *
>> + * 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.

> 
>> + *             val' is val with ',,' replaced by ','
>> + *   and only R may be empty.
>> + *
>> + *   Duplicate keys are permitted; all but the last one are ignored.
>> + *
>> + *   The equations must have a solution.  Counter-example: a.b=1,a=2
>> + *   doesn't have one, because R.a must be an object to satisfy a.b=1
>> + *   and a string to satisfy a=2.
>> + *
>> + * The length of any key-fragment must be between 1 and 127.
>> + *
>> + * Design flaw: there is no way to denote an empty non-root object.
>> + * While interpreting "key absent" as empty object seems natural
>> + * (removing a key-val from the input string removes the member when
>> + * there are more, so why not when it's the last), it doesn't work:
>> + * "key absent" already means "optional object absent", which isn't
>> + * the same as "empty object present".
>> + *
>> + * Additional syntax for use with an implied key:
>> + *
>> + *   key-vals-ik  = val-no-key [ ',' key-vals ]
>> + *   val-no-key   = / [^,]* /

I think this needs to be [^,=]*, since the presence of an = means you've
supplied a key, and are not using the implied-key sugar.

>> + *
>> + * where no-key is syntactic sugar for implied-key=val-no-key.
> 
> s/no-key/val-no-key/ ?
> 
>> + *
>> + * TODO support lists
>> + * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
> 
> Worth another TODO comment for implied values that contain a comma? The
> current restriction feels a bit artificial.

It may be a bit artificial, but at least we can document it: implied
keys are sugar that can only be used for certain values, but you can
always avoid the sugar and explicitly provide the key=value for
problematic values that can't be done with the implied key.


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

signature.asc
Description: OpenPGP digital signature