[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 24/24] keyval: Support lists
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 24/24] keyval: Support lists |
Date: |
Tue, 28 Feb 2017 22:04:11 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 02/28/2017 01:25 PM, Kevin Wolf wrote:
>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>> Additionally permit non-negative integers as key components. A
>>> dictionary's keys must either be all integers or none. If all keys
>>> are integers, convert the dictionary to a list. The set of keys must
>>> be [0,N].
>>>
>
>>> @@ -34,16 +36,16 @@
>>> * doesn't have one, because R.a must be an object to satisfy a.b=1
>>> * and a string to satisfy a=2.
>>> *
>>> - * Key-fragments must be valid QAPI names.
>>> + * Key-fragments must be valid QAPI names or consist only of digits.
>
>>> /*
>>> + * Convert @key to a list index.
>>> + * Convert all leading digits to a (non-negative) number, capped at
>>> + * INT_MAX.
>>> + * If @end is non-null, assign a pointer to the first character after
>>> + * the number to address@hidden
>>> + * Else, fail if any characters follow.
>>> + * On success, return the converted number.
>>> + * On failure, return a negative value.
>>> + * Note: since only digits are converted, no two keys can map to the
>>> + * same number, except by overflow to INT_MAX.
>>> + */
>>> +static int key_to_index(const char *key, const char **end)
>>> +{
>>> + int ret;
>>> + unsigned long index;
>>> +
>>> + if (*key < '0' || *key > '9') {
>>> + return -EINVAL;
>>> + }
>
> So no leading whitespace, '+', or '-', even if strtoul would have
> allowed it. Such names are also invalid as member id names. (There's
> still the question if we want to allow arbitrary whitespace after
> between-key-value ',', and maybe even after between-key-segment '.'
> after this series, to make it easier to write strategically line-wrapped
> command lines - but that's an independent thing to be done on top).
Yes.
>>> @@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict,
>>> const char *params,
>>> cur = qdict;
>>> s = key;
>>> for (;;) {
>>> - ret = parse_qapi_name(s, false);
>>> - len = ret < 0 ? 0 : ret;
>>> + /* Want a key index (unless it's first) or a QAPI name */
>>> + if (s != key && key_to_index(s, &end) >= 0) {
>>> + len = end - s;
>>> + } else {
>>> + ret = parse_qapi_name(s, false);
>>> + len = ret < 0 ? 0 : ret;
>>> + }
>
> Does this mishandle keyval_parse(string, "0", err) - where we want to
> assert that the caller always passes only a valid id name for an
> implicit key?
Crashes and burns...
>>> assert(s + len <= key_end);
>>> if (!len || (s + len < key_end && s[len] != '.')) {
... right here (I tried):
>>> assert(key != implied_key);
>>> @@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict,
>>> const char *params,
>>> return s;
>>> }
>>>
>>> +static char *reassemble_key(GSList *key)
>>> +{
>>> + GString *s = g_string_new("");
>>> + GSList *p;
>>> +
>>> + for (p = key; p; p = p->next) {
>>> + g_string_prepend_c(s, '.');
>>> + g_string_prepend(s, (char *)p->data);
>
> Should this use the canonical form of an index, even if the user spelled
> it with extra bytes like leading zero?
I feel it's best to echo it back to the user exactly how he wrote it, as
far as practical.
>>> + /* Copy @cur's values to @elt[] */
>>> + nelt = qdict_size(cur);
>>> + elt = g_new0(QObject *, nelt);
>>
>> This doesn't seem to be freed.
>>
>>> + for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
>>> + index = key_to_index(ent->key, NULL);
>>> + assert(index >= 0);
>>> + /*
>>> + * We iterate @nelt times. Because the dictionary keys are
>>> + * distinct, the indexes are also distinct (key_to_index()
>>> + * ensures it).
>>
>> Really? What about leading zeros?
>
> key_to_index() should be used to convert "01" and "1" into the same key
> when first computing the QDict during the initial parse; this post-pass
> should thus only see a single "1" key (last-one-wins semantics,
> regardless of the difference in spelling of the same index repeated on
> the command line).
Re: [Qemu-block] [PATCH 00/24] block: Command line option -blockdev, Eric Blake, 2017/02/28