Re: [Qemu-block] [PATCH 24/24] keyval: Support lists

[Eric Blake]

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

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

>>          assert(s + len <= key_end);
>>          if (!len || (s + len < key_end && s[len] != '.')) {
>>              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?


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


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

signature.asc
Description: OpenPGP digital signature