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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9478 bytes --]

On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> 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.
> 

> 
> 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' => [

s/=>/:/

>      { 'bar': 'one', 'wizz': '1' }

s/$/,/

>      { 'bar': 'two', 'wizz': '2' }
>    ],
>  }
> 

> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nested

s/the nested/the nesting/

> used when the same object is defined over QMP.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |   1 +
>  qobject/qdict.c          | 267 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/check-qdict.c      | 143 +++++++++++++++++++++++++
>  3 files changed, 411 insertions(+)
> 
> +
> +/**
> + * qdict_split_flat_key:
> + *
> + * 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.
> + */

Might be worth mentioning that prefix and suffix must both be non-NULL,
and that the caller must g_free() the two resulting strings.

> +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) == '.');

I'd probably have written separator[1] == '.', but your approach is
synonymous.

> +
> +    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] == '.' &&
> +            (*prefix)[i + 1] == '.') {

Technically, if (*prefix)[i] == '.', we could assert((*prefix)[i + 1] ==
'.'), since the only way to get a '.' in prefix is via escaping.  For
that matter, you could short-circuit (part of) the loop by doing a
strchr for '.' (if not found, the loop is not needed; if found, start
the reduction at that point rather on the bytes leading up to that point).

> +            i++;
> +        }
> +        (*prefix)[j] = (*prefix)[i];
> +    }
> +    (*prefix)[j] = '\0';
> +}
> +
> +
> +/**
> + * qdict_list_size:
> + * @maybe_List: dict that may be only list elements

s/List/list/

> + *
> + * Determine whether all keys in @maybe_list are
> + * valid list elements. They they are all valid,

s/They they/If they/

> + * 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.
> + *
> + * Returns: number of list elemets, 0 if a dict, -1 on error

s/elemets/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);
> +
> +        if (qemu_strtoll(entry->key, NULL, 10, &val) == 0) {
> +            if (is_list == -1) {
> +                is_list = 1;
> +            } else if (!is_list) {
> +                error_setg(errp,
> +                           "Key '%s' is for a list, but previous key is "
> +                           "for a dict", entry->key);

Keys are unsorted, so it's a bit hard to call it "previous key".  Maybe
a better error message would be along the lines of "cannot crumple
dictionary because of a mix of list and non-list keys"?  I dunno...

> +                return -1;
> +            }
> +            len++;
> +            if (val > max) {
> +                max = val;
> +            }
> +        } else {
> +            if (is_list == -1) {
> +                is_list = 0;
> +            } else if (is_list) {
> +                error_setg(errp,
> +                           "Key '%s' is for a dict, but previous key is "
> +                           "for a list", entry->key);

...same argument. If we can wordsmith something that makes sense, it
might work for both places.  Otherwise, I can live with your messages.


> +/**
> + * qdict_crumple:
> + *

Worth documenting the 'recursive' parameter?

> + * Reverses the flattening done by qdict_flatten by
> + * crumpling the dicts into a nested structure. Similar
> + * qdict_array_split, but copes with arbitrary nesting
> + * of dicts & arrays, not merely one level of arrays
> + *
> + * { 'foo.0.bar': 'one', 'foo.0.wizz': '1',
> + *   'foo.1.bar': 'two', 'foo.1.wizz': '2' }
> + *
> + * =>
> + *
> + * {
> + *   'foo' => [

s/=>/:/

> + *      { 'bar': 'one', 'wizz': '1' }

s/$/,/

> + *      { 'bar': 'two', 'wizz': '2' }
> + *   ],
> + * }
> + *

Worth mentioning the escaping of '.' in key names?

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

> +
> +    /* Step 2: optionally process the two level dict recursively
> +     * into a multi-level dict */
> +    if (recursive) {

> +
> +    /* 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);
> +            if (!child) {
> +                error_setg(errp, "Unexpected missing list entry %zu", i);

Couldn't we assert() this, since it is a programming bug if
qdict_list_size() let us get this far but then the key disappeared?

Overall looks like it does the trick.


> +++ b/tests/check-qdict.c
> @@ -596,6 +596,140 @@ static void qdict_join_test(void)
>      QDECREF(dict2);
>  }
>  
> +
> +static void qdict_crumple_test_nonrecursive(void)
> +{

This only covers a single layer of collapse, but not turning a dict into
a list.  Is it also worth covering a case where no list indices are
involved, such as the four keys "a.b.d", "a.b.e", "a.c.d", "a.d.e" being
crumpled non-recursively into a single dict "a" with keys "b.d", "b.e",
"c.d", and "d.e"?

> +
> +static void qdict_crumple_test_recursive(void)
> +{
> +

This only covers a list of dict collapse, not a true multi-layer dict
collapse.  Is it also worth covering the same four keys as above, but
this time that dict "a" has keys "b" and "c", each of which is a dict in
turn with keys "d" and "e"?

> +static void qdict_crumple_test_empty(void)
> +{

So an empty dict is never crumpled to an empty list.  I guess that
shouldn't matter.

> +
> +static void qdict_crumple_test_bad_inputs(void)
> +{
> +    QDict *src;
> +    Error *error = NULL;
> +

> +
> +    src = qdict_new();
> +    /* The input should be flat, ie no dicts or lists */
> +    qdict_put(src, "rule.0", qdict_new());
> +    qdict_put(src, "rule.a", qstring_from_str("allow"));

I'd use "rule.a" and "rule.b" here, so that you aren't confusing this
with the earlier test that you can't mix list and dict.

I'd also add a negative test for "rule.1" without "rule.0" being invalid
(missing a list index).

I'll wait to give R-b until I get further into the series, and/or you
post a v4, but it's mostly there.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]