Re: [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()

[Eric Blake]

On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> qdict_flatten(): For each nested QDict with key x, all fields with key y
> are moved to this QDict and their key is renamed to "x.y". This operation
> is applied recursively for nested QDicts.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 50 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)


> +    while (entry != NULL) {
> +
> +        next = qdict_next(qdict, entry);

next points to a position in the unmodified qdict...

> +        value = qdict_entry_value(entry);
> +        new_key = NULL;
> +        delete = false;
> +
> +        if (prefix) {
> +            qobject_incref(value);
> +            new_key = g_strdup_printf("%s.%s", prefix, entry->key);
> +            qdict_put_obj(target, new_key, value);
> +            delete = true;
> +        }
> +
> +        if (qobject_type(value) == QTYPE_QDICT) {
> +            qdict_do_flatten(qobject_to_qdict(value), target,
> +                             new_key ? new_key : entry->key);
> +            delete = true;
> +        }
> +
> +        if (delete) {
> +            qdict_del(qdict, entry->key);
> +
> +            /* Restart loop after modifying the iterated QDict */
> +            entry = qdict_first(qdict);

...now entry points to the head of the modified qdict, since the
modification may have re-arranged where we would iterate next...

> +        }
> +
> +        entry = next;

...oops, we just undid that, and pointed it back to the old qdict
iteration location.  I think you're missing a continue statement inside
'if (delete)'.

If you agree with my analysis and incorporate my suggested fix, then I'm
okay if you add:

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature