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

[Kevin Wolf <kwolf@redhat.com>]

Am 28.02.2017 um 23:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.02.2017 um 22:27 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].
> >> 
> >> Examples:
> >> 
> >> * list.1=goner,list.0=null,list.1=eins,list.2=zwei
> >>   is equivalent to JSON [ "null", "eins", "zwei" ]
> >> 
> >> * a.b.c=1,a.b.0=2
> >>   is inconsistent: a.b.c clashes with a.b.0
> >> 
> >> * list.0=null,list.2=eins,list.2=zwei
> >>   has a hole: list.1 is missing
> >> 
> >> Similar design flaw as for objects: there is no way to denote an empty
> >> list.  While interpreting "key absent" as empty list seems natural
> >> (removing a list member from the input string works when there are
> >> multiple ones, so why not when there's just one), it doesn't work:
> >> "key absent" already means "optional list absent", which isn't the
> >> same as "empty list present".
> >> 
> >> Update the keyval object visitor to use this a.0 syntax in error
> >> messages rather than the usual a[0].
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> >> +/*
> >> + * Listify @cur recursively.
> >> + * Replace QDicts whose keys are all valid list indexes by QLists.
> >> + * @key_of_cur is the list of key fragments leading up to @cur.
> >> + * On success, return either @cur or its replacement.
> >> + * On failure, store an error through @errp and return NULL.
> >> + */
> >> +static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
> >> +{
> >> +    GSList key_node;
> >> +    bool has_index, has_member;
> >> +    const QDictEntry *ent;
> >> +    QDict *qdict;
> >> +    QObject *val;
> >> +    char *key;
> >> +    size_t nelt;
> >> +    QObject **elt;
> >> +    int index, max_index, i;
> >> +    QList *list;
> >> +
> >> +    key_node.next = key_of_cur;
> >> +
> >> +    /*
> >> +     * Recursively listify @cur's members, and figure out whether @cur
> >> +     * itself is to be listified.
> >> +     */
> >> +    has_index = false;
> >> +    has_member = false;
> >> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
> >> +        if (key_to_index(ent->key, NULL) >= 0) {
> >> +            has_index = true;
> >> +        } else {
> >> +            has_member = true;
> >> +        }
> >> +
> >> +        qdict = qobject_to_qdict(ent->value);
> >> +        if (!qdict) {
> >> +            continue;
> >> +        }
> >> +
> >> +        key_node.data = ent->key;
> >> +        val = keyval_listify(qdict, &key_node, errp);
> >> +        if (!val) {
> >> +            return NULL;
> >> +        }
> >> +        if (val != ent->value) {
> >> +            qdict_put_obj(cur, ent->key, val);
> >> +        }
> >> +    }
> >> +
> >> +    if (has_index && has_member) {
> >> +        key = reassemble_key(key_of_cur);
> >> +        error_setg(errp, "Parameters '%s*' used inconsistently", key);
> >> +        g_free(key);
> >> +        return NULL;
> >> +    }
> >> +    if (!has_index) {
> >> +        return QOBJECT(cur);
> >> +    }
> >> +
> >> +    /* Copy @cur's values to @elt[] */
> >> +    nelt = qdict_size(cur) + 1; /* one extra, for use as sentinel */
> >> +    elt = g_new0(QObject *, nelt);
> >> +    max_index = -1;
> >> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
> >> +        index = key_to_index(ent->key, NULL);
> >> +        assert(index >= 0);
> >> +        if (index > max_index) {
> >> +            max_index = index;
> >> +        }
> >> +        /*
> >> +         * We iterate @nelt times.  If we get one exceeding @nelt
> >> +         * here, we will put less than @nelt values into @elt[],
> >> +         * triggering the error in the next loop.
> >> +         */
> >> +        if ((size_t)index >= nelt) {
> >
> > I think this needs to be index >= nelt - 1 now...
> >
> >> +            continue;
> >> +        }
> >> +        /* Even though dict keys are distinct, indexes need not be */
> >> +        elt[index] = ent->value;
> >> +    }
> >> +
> >> +    /*
> >> +     * Make a list from @elt[], reporting any missing elements.
> >> +     * If we dropped an index >= nelt in the previous loop, this loop
> >> +     * will run into the sentinel and report index @nelt missing.
> >> +     */
> >> +    list = qlist_new();
> >> +    assert(!elt[nelt-1]);       /* need the sentinel to be null */
> >
> > ...or this assertion can fail.
> 
> Yup.
> 
> Appended fix squashed in and pushed to my public repo.  R-by?
> 
> >> +    for (i = 0; i < MIN(nelt, max_index + 1); i++) {
> >> +        if (!elt[i]) {
> >> +            key = reassemble_key(key_of_cur);
> >> +            error_setg(errp, "Parameter '%s%d' missing", key, i);
> >> +            g_free(key);
> >> +            g_free(elt);
> >> +            QDECREF(list);
> >> +            return NULL;
> >> +        }
> >> +        qobject_incref(elt[i]);
> >> +        qlist_append_obj(list, elt[i]);
> >> +    }
> >> +
> >> +    g_free(elt);
> >> +    return QOBJECT(list);
> >> +}
> >
> > Kevin
> 
> 
> diff --git a/util/keyval.c b/util/keyval.c
> index b57a742..c316aaa 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -329,7 +329,7 @@ static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
>           * here, we will put less than @nelt values into @elt[],
>           * triggering the error in the next loop.
>           */
> -        if ((size_t)index >= nelt) {
> +        if ((size_t)index >= nelt - 1) {
>              continue;
>          }
>          /* Even though dict keys are distinct, indexes need not be */

With this squashed in:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>