Re: [Qemu-devel] [PATCH v2 06/10] qdict: Make qdict_flatten() shallow-clone-friendly

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> In its current form, qdict_flatten() removes all entries from nested
> QDicts that are moved to the root QDict.  It is completely sufficient to
> remove all old entries from the root QDict, however.  If the nested
> dicts have a refcount of 1, this will automatically delete them, too.
> And if they have a greater refcount, we probably do not want to modify
> them in the first place.
>
> The latter observation means that it was currently (in general)
> impossible to qdict_flatten() a shallowly cloned dict because that would
> empty nested QDicts in the original dict as well.  This patch changes
> this, so you can now use qdict_flatten(qdict_shallow_clone(dict)) to get
> a flattened copy without disturbing the original.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  qobject/block-qdict.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> index 0b2ae02627..7e0deb74c5 100644
> --- a/qobject/block-qdict.c
> +++ b/qobject/block-qdict.c
> @@ -114,19 +114,30 @@ static void qdict_flatten_qdict(QDict *qdict, QDict 
> *target, const char *prefix)
>  
>          /*
>           * Flatten non-empty QDict and QList recursively into @target,
> -         * copy other objects to @target
> +         * copy other objects to @target.
> +         * On the root level (if @qdict == @target), remove flattened
> +         * nested QDicts and QLists from @qdict.
> +         *
> +         * (Note that we do not need to remove entries from nested
> +         * dicts or lists.  Their reference count is decremented on
> +         * the root level, so there are no leaks.  In fact, if they
> +         * have a reference count greater than one, we are probably
> +         * well advised not to modify them altogether.)
>           */
>          if (dict_val && qdict_size(dict_val)) {
>              qdict_flatten_qdict(dict_val, target,
>                                  new_key ? new_key : entry->key);
> -            qdict_del(qdict, entry->key);
> +            if (target == qdict) {
> +                qdict_del(qdict, entry->key);
> +            }
>          } else if (list_val && !qlist_empty(list_val)) {
>              qdict_flatten_qlist(list_val, target,
>                                  new_key ? new_key : entry->key);
> -            qdict_del(qdict, entry->key);
> +            if (target == qdict) {
> +                qdict_del(qdict, entry->key);
> +            }
>          } else if (target != qdict) {
>              qdict_put_obj(target, new_key, qobject_ref(value));
> -            qdict_del(qdict, entry->key);
>          }
>  
>          g_free(new_key);

This simply makes sense, even without the later patch(es) that rely on
it.

Reviewed-by: Markus Armbruster <address@hidden>