Re: [Qemu-devel] [PATCH v5 01/11] qdict: implement a qdict_crumple method for un-flattening a dict

[Markus Armbruster]

I apologize for the lateness of this review.

"Daniel P. Berrange" <address@hidden> writes:

> The qdict_flatten() method will take a dict whose elements are
> further nested dicts/lists and flatten them by concatenating
> keys.
>
> The qdict_crumple() method aims to do the reverse, taking a flat
> qdict, and turning it into a set of nested dicts/lists. It will
> apply nesting based on the key name, with a '.' indicating a
> new level in the hierarchy. If the keys in the nested structure
> are all numeric, it will create a list, otherwise it will create
> a dict.
>
> If the keys are a mixture of numeric and non-numeric, or the
> numeric keys are not in strictly ascending order, an error will
> be reported.
>
> As an example, a flat dict containing
>
>  {
>    'foo.0.bar': 'one',
>    'foo.0.wizz': '1',
>    'foo.1.bar': 'two',
>    'foo.1.wizz': '2'
>  }
>
> will get turned into a dict with one element 'foo' whose
> value is a list. The list elements will each in turn be
> dicts.
>
>  {
>    'foo': [
>      { 'bar': 'one', 'wizz': '1' },
>      { 'bar': 'two', 'wizz': '2' }
>    ],
>  }
>
> If the key is intended to contain a literal '.', then it must
> be escaped as '..'. ie a flat dict
>
>   {
>      'foo..bar': 'wizz',
>      'bar.foo..bar': 'eek',
>      'bar.hello': 'world'
>   }
>
> Will end up as
>
>   {
>      'foo.bar': 'wizz',
>      'bar': {
>         'foo.bar': 'eek',
>         'hello': 'world'
>      }
>   }
>
> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nesting
> used when the same object is defined over QMP.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>

Not an objection to your patch, just an observation: we're digging
ourselves ever deeper into the QemuOpts / QDict hole.  We've pushed
QemuOpts far beyond its original purpose "parse key=value,... option
arguments".  In my opinion, we'd better parse straight to QAPI-generated
structs.

> ---
>  include/qapi/qmp/qdict.h |   1 +
>  qobject/qdict.c          | 282 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/check-qdict.c      | 229 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 512 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 71b8eb0..8a3ac13 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  int qdict_array_entries(QDict *src, const char *subqdict);
> +QObject *qdict_crumple(QDict *src, bool recursive, Error **errp);
>  
>  void qdict_join(QDict *dest, QDict *src, bool overwrite);
>  
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 60f158c..ad6a563 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -17,6 +17,7 @@
>  #include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/qmp/qobject.h"
> +#include "qapi/error.h"
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
> @@ -683,6 +684,287 @@ void qdict_array_split(QDict *src, QList **dst)
>      }
>  }
>  
> +
> +/**
> + * qdict_split_flat_key:
> + * @key: the key string to split
> + * @prefix: non-NULL pointer to hold extracted prefix
> + * @suffix: non-NULL pointer to hold extracted suffix
> + *
> + * Given a flattened key such as 'foo.0.bar', split it
> + * into two parts at the first '.' separator. Allows
> + * double dot ('..') to escape the normal separator.
> + *
> + * eg
> + *    'foo.0.bar' -> prefix='foo' and suffix='0.bar'
> + *    'foo..0.bar' -> prefix='foo.0' and suffix='bar'
> + *
> + * The '..' sequence will be unescaped in the returned
> + * 'prefix' string. The 'suffix' string will be left
> + * in escaped format, so it can be fed back into the
> + * qdict_split_flat_key() key as the input later.

Why is the suffix strdup'ed then?

> + *
> + * The caller is responsible for freeing the strings
> + * returned in @prefix and @suffix using g_free().

Wrap comment lines around column 70, please.

> + */
> +static void qdict_split_flat_key(const char *key, char **prefix, char 
> **suffix)
> +{
> +    const char *separator;
> +    size_t i, j;
> +
> +    /* Find first '.' separator, but if there is a pair '..'
> +     * that acts as an escape, so skip over '..' */
> +    separator = NULL;
> +    do {
> +        if (separator) {
> +            separator += 2;
> +        } else {
> +            separator = key;
> +        }
> +        separator = strchr(separator, '.');
> +    } while (separator && separator[1] == '.');
> +
> +    if (separator) {
> +        *prefix = g_strndup(key,
> +                            separator - key);
> +        *suffix = g_strdup(separator + 1);
> +    } else {
> +        *prefix = g_strdup(key);
> +        *suffix = NULL;
> +    }
> +
> +    /* Unescape the '..' sequence into '.' */
> +    for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) {
> +        if ((*prefix)[i] == '.') {
> +            assert((*prefix)[i + 1] == '.');
> +            i++;
> +        }
> +        (*prefix)[j] = (*prefix)[i];
> +    }
> +    (*prefix)[j] = '\0';

Runs through the key twice, once in g_strndup(), and once here.  Would
be easy enough to avoid, but since this function isn't performance
critical, it's okay as is.

> +}
> +
> +
> +/**
> + * qdict_list_size:
> + * @maybe_list: dict that may be only list elements

Huh?  How can a dictionary "be only list elements"?

Do you mean "the dictionary to test?"

> + *
> + * Determine whether all keys in @maybe_list are
> + * valid list elements. If they are all valid,
> + * then this returns the number of elements. If
> + * they all look like non-numeric keys, then returns
> + * zero. If there is a mix of numeric and non-numeric
> + * keys, then an error is set as it is both a list
> + * and a dict at once.

This is well-defined only if empty @maybe_list is considered to have
dict nature, not list nature.  Else, return value zero could be the
length of the empty list or the special "has dict nature" value.

Please spell out behavior for empty @maybe_list.

> + *
> + * Returns: number of list elements, 0 if a dict, -1 on error

Awkward function name.  qdict_list_size_if_list() would be clear.

But I'd simply turn this into a predicate qdict_is_list(), and have the
caller use qdict_size() to get the number of elements.

> + */
> +static ssize_t qdict_list_size(QDict *maybe_list, Error **errp)
> +{
> +    const QDictEntry *entry, *next;
> +    ssize_t len = 0;
> +    ssize_t max = -1;
> +    int is_list = -1;
> +    int64_t val;
> +
> +    entry = qdict_first(maybe_list);
> +    while (entry != NULL) {
> +        next = qdict_next(maybe_list, entry);

Please keep the loop control in one place:

       for (entry = qdict_first(maybe_list); entry; entry = qdict_next(entry)) {

I'd rename some variables for less verbiage:

       for (ent = qdict_first(dict); ent; ent = qdict_next(ent)) {

Your loop control also works when the loop body deletes @entry from
@maybe_list.  Seeing such loop control in a function that isn't supposed
to change the its argument makes the reviewer go "WTF?!?" :)

> +
> +        if (qemu_strtoll(entry->key, NULL, 10, &val) == 0) {
> +            if (is_list == -1) {
> +                is_list = 1;
> +            } else if (!is_list) {
> +                error_setg(errp,
> +                           "Cannot crumple a dictionary with a mix of list "
> +                           "and non-list keys");
> +                return -1;
> +            }
> +            len++;
> +            if (val > max) {
> +                max = val;
> +            }
> +        } else {
> +            if (is_list == -1) {
> +                is_list = 0;
> +            } else if (is_list) {
> +                error_setg(errp,
> +                           "Cannot crumple a dictionary with a mix of list "
> +                           "and non-list keys");
> +                return -1;
> +            }
> +        }
> +
> +        entry = next;
> +    }
> +
> +    if (is_list == -1) {
> +        is_list = 0;

This can happen only when @maybe_list is empty.  Okay, but perhaps you'd
like to assert(!qdict_size(maybe_list)).

> +    }
> +
> +    if (len != (max + 1)) {
> +        error_setg(errp, "List indexes are not continuous, "
> +                   "saw %zd elements but %zd largest index",
> +                   len, max);
> +        return -1;

contiguous?

What if we saw indexes 0, 2, 2?

Hmm, see [*] below.

> +    }
> +
> +    return is_list ? len : 0;
> +}
> +
> +/**
> + * qdict_crumple:
> + * @src: the original flat dictionary to crumple

"Flat" means all values are scalar.  Should we spell that out?

> + * @recursive: true to recursively crumple nested dictionaries
> + *
> + * Takes a flat dictionary whose keys use '.' separator to
> + * indicate nesting, and values are scalars, crumplings it

s/, crumplings/, and crumples/

> + * into a nested structure. If the @recursive parameter is
> + * false, then only the first level of structure implied
> + * by the keys will be crumpled. If @recursive is true,
> + * then the input will be recursively crumpled  to expand
> + * all levels of structure in the keys.
> + *
> + * To include a literal '.' in a key name, it must be escaped
> + * as '..'
> + *
> + * For example, an input of:
> + *
> + * { 'foo.0.bar': 'one', 'foo.0.wizz': '1',
> + *   'foo.1.bar': 'two', 'foo.1.wizz': '2' }
> + *
> + * will result in any output of:
> + *
> + * {
> + *   'foo': [
> + *      { 'bar': 'one', 'wizz': '1' },
> + *      { 'bar': 'two', 'wizz': '2' }
> + *   ],
> + * }
> + *
> + * Returns: either a QDict or QList for the nested data structure

I think you should discuss how this can fail.

> + */
> +QObject *qdict_crumple(QDict *src, bool recursive, Error **errp)
> +{
> +    const QDictEntry *entry, *next;
> +    QDict *two_level, *multi_level = NULL;
> +    QObject *dst = NULL, *child;
> +    ssize_t list_len;
> +    size_t i;
> +    char *prefix = NULL, *suffix = NULL;
> +
> +    two_level = qdict_new();
> +    entry = qdict_first(src);
> +
> +    /* Step 1: split our totally flat dict into a two level dict */
> +    while (entry != NULL) {
> +        next = qdict_next(src, entry);

Again, keep the loop control in one place.

> +
> +        if (qobject_type(entry->value) == QTYPE_QDICT ||
> +            qobject_type(entry->value) == QTYPE_QLIST) {
> +            error_setg(errp, "Value %s is not a scalar",
> +                       entry->key);
> +            goto error;
> +        }
> +
> +        qdict_split_flat_key(entry->key, &prefix, &suffix);
> +
> +        child = qdict_get(two_level, prefix);
> +        if (suffix) {
> +            if (child) {
> +                if (qobject_type(child) != QTYPE_QDICT) {
> +                    error_setg(errp, "Key %s prefix is already set as a 
> scalar",
> +                               prefix);
> +                    goto error;
> +                }
> +            } else {
> +                child = QOBJECT(qdict_new());
> +                qdict_put_obj(two_level, prefix, child);
> +            }
> +            qobject_incref(entry->value);
> +            qdict_put_obj(qobject_to_qdict(child), suffix, entry->value);
> +        } else {
> +            if (child) {
> +                error_setg(errp, "Key %s prefix is already set as a dict",
> +                           prefix);
> +                goto error;
> +            }
> +            qobject_incref(entry->value);
> +            qdict_put_obj(two_level, prefix, entry->value);
> +        }

Works, because we put only QDicts we've created ourselves (first
qdict_put_obj() above) and values we got from @src (second
qdict_put_obj()), and we fail when such a value isn't scalar.

> +
> +        g_free(suffix);

As I suspected, qdict_split_flat_key() strdup'ing the suffix is useless.

> +        g_free(prefix);
> +        suffix = prefix = NULL;

Dead stores.

> +
> +        entry = next;
> +    }
> +
> +    /* Step 2: optionally process the two level dict recursively
> +     * into a multi-level dict */
> +    if (recursive) {
> +        multi_level = qdict_new();
> +        entry = qdict_first(two_level);
> +        while (entry != NULL) {
> +            next = qdict_next(two_level, entry);

Again, keep the loop control in one place.

> +
> +            if (qobject_type(entry->value) == QTYPE_QDICT) {
> +                child = qdict_crumple(qobject_to_qdict(entry->value),
> +                                      recursive, errp);
> +                if (!child) {
> +                    goto error;
> +                }
> +
> +                qdict_put_obj(multi_level, entry->key, child);
> +            } else {
> +                qobject_incref(entry->value);
> +                qdict_put_obj(multi_level, entry->key, entry->value);
> +            }
> +
> +            entry = next;
> +        }
> +        QDECREF(two_level);
> +    } else {
> +        multi_level = two_level;
> +    }
> +    two_level = NULL;
> +
> +    /* Step 3: detect if we need to turn our dict into list */
> +    list_len = qdict_list_size(multi_level, errp);
> +    if (list_len < 0) {
> +        goto error;
> +    }
> +
> +    if (list_len) {
> +        dst = QOBJECT(qlist_new());
> +
> +        for (i = 0; i < list_len; i++) {
> +            char *key = g_strdup_printf("%zu", i);
> +
> +            child = qdict_get(multi_level, key);
> +            g_free(key);
> +            assert(child);

qdict_list_size() accepts as list index any (string) key qemu_strtoll()
accepts.  If %zu formats it back into the same string, we'll find it
here.  Else we die.  Please spell this out in the function contract.

[*] I'm afraid we also die if qdict_list_size()'s "List indexes are not
continuous" check gets fooled.  Suggest to drop that check, and replace
this assertion by error_setg(errp, "Malformed list indexes").
Admittedly not the nicest error message; perhaps you can come up with a
better one.

> +
> +            qobject_incref(child);
> +            qlist_append_obj(qobject_to_qlist(dst), child);
> +        }
> +        QDECREF(multi_level);

Do we need

           multi_level = NULL;

here?

> +    } else {
> +        dst = QOBJECT(multi_level);
> +    }
> +
> +    return dst;
> +
> + error:
> +    g_free(suffix);
> +    g_free(prefix);
> +    QDECREF(multi_level);
> +    QDECREF(two_level);
> +    qobject_decref(dst);
> +    return NULL;
> +}
> +
> +
>  /**
>   * qdict_array_entries(): Returns the number of direct array entries if the
>   * sub-QDict of src specified by the prefix in subqdict (or src itself for
> diff --git a/tests/check-qdict.c b/tests/check-qdict.c
> index a43056c..0d12f40 100644
> --- a/tests/check-qdict.c
> +++ b/tests/check-qdict.c
> @@ -15,6 +15,7 @@
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/error.h"
>  #include "qemu-common.h"
>  
>  /*
> @@ -596,6 +597,223 @@ static void qdict_join_test(void)
>      QDECREF(dict2);
>  }
>  
> +
> +static void qdict_crumple_test_nonrecursive(void)
> +{
> +    QDict *src, *dst, *rules, *vnc;
> +    QObject *child, *res;
> +
> +    src = qdict_new();
> +    qdict_put(src, "vnc.listen.addr", qstring_from_str("127.0.0.1"));
> +    qdict_put(src, "vnc.listen.port", qstring_from_str("5901"));
> +    qdict_put(src, "rule.0.match", qstring_from_str("fred"));
> +    qdict_put(src, "rule.0.policy", qstring_from_str("allow"));
> +    qdict_put(src, "rule.1.match", qstring_from_str("bob"));
> +    qdict_put(src, "rule.1.policy", qstring_from_str("deny"));
> +
> +    res = qdict_crumple(src, false, &error_abort);
> +
> +    g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
> +
> +    dst = qobject_to_qdict(res);
> +
> +    g_assert_cmpint(qdict_size(dst), ==, 2);
> +
> +    child = qdict_get(dst, "vnc");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
> +    vnc = qdict_get_qdict(dst, "vnc");
> +
> +    g_assert_cmpint(qdict_size(vnc), ==, 2);
> +
> +    g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(vnc, "listen.addr"));
> +    g_assert_cmpstr("5901", ==, qdict_get_str(vnc, "listen.port"));
> +
> +    child = qdict_get(dst, "rule");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
> +    rules = qdict_get_qdict(dst, "rule");
> +
> +    g_assert_cmpint(qdict_size(rules), ==, 4);
> +
> +    g_assert_cmpstr("fred", ==, qdict_get_str(rules, "0.match"));
> +    g_assert_cmpstr("allow", ==, qdict_get_str(rules, "0.policy"));
> +    g_assert_cmpstr("bob", ==, qdict_get_str(rules, "1.match"));
> +    g_assert_cmpstr("deny", ==, qdict_get_str(rules, "1.policy"));
> +
> +    QDECREF(src);
> +    QDECREF(dst);
> +}
> +
> +
> +static void qdict_crumple_test_listtop(void)
> +{
> +    QDict *src, *rule;
> +    QList *rules;
> +    QObject *res;
> +
> +    src = qdict_new();
> +    qdict_put(src, "0.match.name", qstring_from_str("Fred"));
> +    qdict_put(src, "0.match.email", qstring_from_str("address@hidden"));
> +    qdict_put(src, "0.policy", qstring_from_str("allow"));
> +    qdict_put(src, "1.match.name", qstring_from_str("Bob"));
> +    qdict_put(src, "1.match.email", qstring_from_str("address@hidden"));
> +    qdict_put(src, "1.policy", qstring_from_str("deny"));
> +
> +    res = qdict_crumple(src, false, &error_abort);
> +
> +    g_assert_cmpint(qobject_type(res), ==, QTYPE_QLIST);
> +
> +    rules = qobject_to_qlist(res);
> +
> +    g_assert_cmpint(qlist_size(rules), ==, 2);
> +
> +    g_assert_cmpint(qobject_type(qlist_peek(rules)), ==, QTYPE_QDICT);
> +    rule = qobject_to_qdict(qlist_pop(rules));
> +    g_assert_cmpint(qdict_size(rule), ==, 3);
> +
> +    g_assert_cmpstr("Fred", ==, qdict_get_str(rule, "match.name"));
> +    g_assert_cmpstr("address@hidden", ==, qdict_get_str(rule, 
> "match.email"));
> +    g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy"));
> +    QDECREF(rule);
> +
> +    g_assert_cmpint(qobject_type(qlist_peek(rules)), ==, QTYPE_QDICT);
> +    rule = qobject_to_qdict(qlist_pop(rules));
> +    g_assert_cmpint(qdict_size(rule), ==, 3);
> +
> +    g_assert_cmpstr("Bob", ==, qdict_get_str(rule, "match.name"));
> +    g_assert_cmpstr("address@hidden", ==, qdict_get_str(rule, 
> "match.email"));
> +    g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy"));
> +    QDECREF(rule);
> +
> +    QDECREF(src);
> +    qobject_decref(res);
> +}
> +
> +
> +static void qdict_crumple_test_recursive(void)
> +{
> +    QDict *src, *dst, *rule, *vnc, *acl, *listen;
> +    QObject *child, *res;
> +    QList *rules;
> +
> +    src = qdict_new();
> +    qdict_put(src, "vnc.listen.addr", qstring_from_str("127.0.0.1"));
> +    qdict_put(src, "vnc.listen.port", qstring_from_str("5901"));
> +    qdict_put(src, "vnc.acl.rules.0.match", qstring_from_str("fred"));
> +    qdict_put(src, "vnc.acl.rules.0.policy", qstring_from_str("allow"));
> +    qdict_put(src, "vnc.acl.rules.1.match", qstring_from_str("bob"));
> +    qdict_put(src, "vnc.acl.rules.1.policy", qstring_from_str("deny"));
> +    qdict_put(src, "vnc.acl.default", qstring_from_str("deny"));
> +
> +    res = qdict_crumple(src, true, &error_abort);
> +
> +    g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
> +
> +    dst = qobject_to_qdict(res);
> +
> +    g_assert_cmpint(qdict_size(dst), ==, 1);
> +
> +    child = qdict_get(dst, "vnc");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
> +    vnc = qobject_to_qdict(child);
> +
> +    child = qdict_get(vnc, "listen");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
> +    listen = qobject_to_qdict(child);
> +    g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(listen, "addr"));
> +    g_assert_cmpstr("5901", ==, qdict_get_str(listen, "port"));
> +
> +    child = qdict_get(vnc, "acl");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
> +    acl = qobject_to_qdict(child);
> +
> +    child = qdict_get(acl, "rules");
> +    g_assert_cmpint(qobject_type(child), ==, QTYPE_QLIST);
> +    rules = qobject_to_qlist(child);
> +    g_assert_cmpint(qlist_size(rules), ==, 2);
> +
> +    rule = qobject_to_qdict(qlist_pop(rules));
> +    g_assert_cmpint(qdict_size(rule), ==, 2);
> +    g_assert_cmpstr("fred", ==, qdict_get_str(rule, "match"));
> +    g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy"));
> +    QDECREF(rule);
> +
> +    rule = qobject_to_qdict(qlist_pop(rules));
> +    g_assert_cmpint(qdict_size(rule), ==, 2);
> +    g_assert_cmpstr("bob", ==, qdict_get_str(rule, "match"));
> +    g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy"));
> +    QDECREF(rule);
> +
> +    QDECREF(src);
> +    QDECREF(dst);
> +}
> +
> +
> +static void qdict_crumple_test_empty(void)
> +{
> +    QDict *src, *dst;
> +
> +    src = qdict_new();
> +
> +    dst = (QDict *)qdict_crumple(src, true, &error_abort);
> +
> +    g_assert_cmpint(qdict_size(dst), ==, 0);
> +
> +    QDECREF(src);
> +    QDECREF(dst);
> +}
> +
> +
> +static void qdict_crumple_test_bad_inputs(void)
> +{
> +    QDict *src;
> +    Error *error = NULL;
> +
> +    src = qdict_new();
> +    /* rule.0 can't be both a string and a dict */
> +    qdict_put(src, "rule.0", qstring_from_str("fred"));
> +    qdict_put(src, "rule.0.policy", qstring_from_str("allow"));
> +
> +    g_assert(qdict_crumple(src, true, &error) == NULL);
> +    g_assert(error != NULL);
> +    error_free(error);
> +    error = NULL;
> +    QDECREF(src);
> +
> +    src = qdict_new();
> +    /* rule can't be both a list and a dict */
> +    qdict_put(src, "rule.0", qstring_from_str("fred"));
> +    qdict_put(src, "rule.a", qstring_from_str("allow"));
> +
> +    g_assert(qdict_crumple(src, true, &error) == NULL);
> +    g_assert(error != NULL);
> +    error_free(error);
> +    error = NULL;
> +    QDECREF(src);
> +
> +    src = qdict_new();
> +    /* The input should be flat, ie no dicts or lists */
> +    qdict_put(src, "rule.a", qdict_new());
> +    qdict_put(src, "rule.b", qstring_from_str("allow"));
> +
> +    g_assert(qdict_crumple(src, true, &error) == NULL);
> +    g_assert(error != NULL);
> +    error_free(error);
> +    error = NULL;
> +    QDECREF(src);
> +
> +
> +    src = qdict_new();
> +    /* List indexes must not have gaps */
> +    qdict_put(src, "rule.0", qdict_new());
> +    qdict_put(src, "rule.3", qstring_from_str("allow"));
> +
> +    g_assert(qdict_crumple(src, true, &error) == NULL);
> +    g_assert(error != NULL);
> +    error_free(error);
> +    error = NULL;
> +    QDECREF(src);

Suggest to add test case

       /* List indexes must not have gaps (more creative version) */
       qdict_put(src, "rule.0", ...);
       qdict_put(src, "rule.2", ...);
       qdict_put(src, "rule.2", ...);

and

       /* List indexes must be in %zu format */
       qdict_put(src, "rule.+0", ...);

> +}
> +
>  /*
>   * Errors test-cases
>   */
> @@ -743,6 +961,17 @@ int main(int argc, char **argv)
>      g_test_add_func("/errors/put_exists", qdict_put_exists_test);
>      g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test);
>  
> +    g_test_add_func("/public/crumple/nonrecursive",
> +                    qdict_crumple_test_nonrecursive);
> +    g_test_add_func("/public/crumple/recursive",
> +                    qdict_crumple_test_recursive);
> +    g_test_add_func("/public/crumple/listtop",
> +                    qdict_crumple_test_listtop);
> +    g_test_add_func("/public/crumple/empty",
> +                    qdict_crumple_test_empty);
> +    g_test_add_func("/public/crumple/bad_inputs",
> +                    qdict_crumple_test_bad_inputs);
> +
>      /* The Big one */
>      if (g_test_slow()) {
>          g_test_add_func("/stress/test", qdict_stress_test);