Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Stefan Hajnoczi]

On Fri, Dec 20, 2013 at 06:23:26PM +0100, Max Reitz wrote:
> On 20.12.2013 18:19, Max Reitz wrote:
> >On 20.12.2013 10:46, Stefan Hajnoczi wrote:
> >>>diff --git a/qobject/qdict.c b/qobject/qdict.c
> >>>index fca1902..7b6b08a 100644
> >>>--- a/qobject/qdict.c
> >>>+++ b/qobject/qdict.c
> >>>@@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
> >>>      g_free(qdict);
> >>>  }
> >>>  -static void qdict_do_flatten(QDict *qdict, QDict *target,
> >>>const char *prefix)
> >>>+static void qdict_flatten_qdict(QDict *qdict, QDict *target,
> >>>+                                const char *prefix);
> >>>+
> >>>+static void qdict_flatten_qlist(QList *qlist, QDict *target,
> >>>const char *prefix)
> >>>+{
> >>>+    QObject *value;
> >>>+    const QListEntry *entry;
> >>>+    char *new_key;
> >>>+    int i;
> >>>+
> >>>+    /* This function is never called with prefix == NULL,
> >>>i.e., it is always
> >>>+     * called from within qdict_flatten_q(list|dict)().
> >>>Therefore, it does not
> >>>+     * need to remove list entries during the iteration (the
> >>>whole list will be
> >>>+     * deleted eventually anyway from qdict_flatten_qdict()). */
> >>>+    assert(prefix);
> >>>+
> >>>+    entry = qlist_first(qlist);
> >>>+
> >>>+    for (i = 0; entry; entry = qlist_next(entry), i++) {
> >>>+        value = qlist_entry_obj(entry);
> >>>+
> >>>+        qobject_incref(value);
> >>>+        new_key = g_strdup_printf("%s.%i", prefix, i);
> >>>+        qdict_put_obj(target, new_key, value);
> >>It seems this operation is clobbered by what follows and should be
> >>deleted.  Is the incref also superfluous or...
> >>
> >>>+
> >>>+        if (qobject_type(value) == QTYPE_QDICT) {
> >>>+            qdict_flatten_qdict(qobject_to_qdict(value),
> >>>target, new_key);
> >>>+        } else if (qobject_type(value) == QTYPE_QLIST) {
> >>>+            qdict_flatten_qlist(qobject_to_qlist(value),
> >>>target, new_key);
> >>>+        } else {
> >>>+            /* All other types are moved to the target unchanged. */
> >>>+            qobject_incref(value);
> >>...should this one be deleted?
> >
> >Oh great, I've been fixing working code, then. *g*
> >
> >I added the else branch in v5, I guess, it was correct not to have
> >it (however, the condition in the else if was missing in v4). I'll
> >drop it again, thanks.
> 
> On second thought, that would be even more wrong. I'll leave the
> else branch and drop the qobject_incref() and qdict_put_obj() above
> the condition.

I agree, that sounds like the right solution.