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

[Max Reitz]

On 20.12.2013 18:19, Max Reitz wrote:
> On 20.12.2013 10:46, Stefan Hajnoczi wrote:
> > On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote:
> > > Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
> > > well by interpreting them as QDicts where every entry's key is its
> > > index.
> > >
> > > This allows bringing QDicts with QLists from QMP commands to the same
> > > form as they would be given as command-line options, thereby allowing
> > > them to be parsed the same way.
> > >
> > > Signed-off-by: Max Reitz <address@hidden>
> > > ---
> > >   qobject/qdict.c | 57 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++------
> > >   1 file changed, 51 insertions(+), 6 deletions(-)
> > Please add a tests/check-qdict.c test case.
> >
> > > 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.

Max

> > > +            qdict_put_obj(target, new_key, value);
> > > +        }
> > > +
> > > +        g_free(new_key);
> > > +    }