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

[Eric Blake]

On 02/28/2017 03:27 PM, Markus Armbruster wrote:
> 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].
> 

> +static void test_keyval_parse_list(void)
> +{
> +    Error *err = NULL;
> +    QDict *qdict, *sub_qdict;
> +

> +    /* Missing list indexes */
> +    qdict = keyval_parse("list.2=lonely", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, &err);

You have a negative test that covers when list.2 and list.02 map to the
same thing (but we forgot list.1) [good], but no positive test of
leading zeroes still resulting in a correct QList.  Seems like a
reasonable followup patch during freeze (testsuite additions are freeze
material, and I don't want to delay the pull request).

At any rate, this one looks better than v1 at handling leading zero
index duplication.

>  /*
> + * Convert @key to a list index.
> + * Convert all leading digits to a (non-negative) number, capped at

Maybe insert the word 'decimal', to make it obvious that 010 is index
10, not 8?

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

> +
> +    /*
> +     * 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 */
> +    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;

If more than one gap exists, this only reports the first missing
element; "any missing elements" implies that it reports all.  No
different from our behavior on structs, where we only report one
(random!) excess element or the first missing one in visit order, rather
than all problems, so all that needs tweaking would be the comment.

Can all be done as followups, without invalidating the pull request.

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

signature.asc
Description: OpenPGP digital signature