[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse() |
Date: |
Tue, 28 Feb 2017 19:03:31 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
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.
> 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 } [ ',' ]
>>> + * key-val = key '=' val
>>> + * key = key-fragment { '.' key-fragment }
>
> Ambiguous.
I'm dense. Can you give me an example string with two derivations?
>>> + * 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.
>>> + * 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.
We're in a bit of a time squeeze right now. I'd like to clarify the
comment on top.
>>> + * 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.
You're right.
>>> + *
>>> + * 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.
- [Qemu-block] [PATCH 01/24] test-qemu-opts: Cover qemu_opts_parse() of "no", (continued)
[Qemu-block] [PATCH 04/24] qapi: qobject input visitor variant for use with keyval_parse(), Markus Armbruster, 2017/02/27
[Qemu-block] [PATCH 20/24] docs/qapi-code-gen.txt: Clarify naming rules, Markus Armbruster, 2017/02/27
[Qemu-block] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience, Markus Armbruster, 2017/02/27