Eric Blake <ebl...@redhat.com> 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 *@end. >>> + * 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).