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

[Fam Zheng <famz@redhat.com>]

On 2013年12月12日 02:10, 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 <mreitz@redhat.com>
> ---

The logic looks fine, just a few questions about assertions below.

>   qobject/qdict.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index fca1902..d7ce4b3 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -477,7 +477,40 @@ 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()). Also, prefix can
> +     * never be NULL. */

Then maybe assert this?

> +
> +    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);
> +
> +        if (qobject_type(value) == QTYPE_QDICT) {
> +            qdict_flatten_qdict(qobject_to_qdict(value), target, new_key);
> +        } else {

And maybe assert the type are not something other than dict and list?


> +            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
> +        }
> +    }
> +}
> +
> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
>   {
>       QObject *value;
>       const QDictEntry *entry, *next;
> @@ -500,8 +533,12 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
>           if (qobject_type(value) == QTYPE_QDICT) {
>               /* Entries of QDicts are processed recursively, the QDict object
>                * itself disappears. */
> -            qdict_do_flatten(qobject_to_qdict(value), target,
> -                             new_key ? new_key : entry->key);
> +            qdict_flatten_qdict(qobject_to_qdict(value), target,
> +                                new_key ? new_key : entry->key);
> +            delete = true;
> +        } else if (qobject_type(value) == QTYPE_QLIST) {
> +            qdict_flatten_qlist(qobject_to_qlist(value), target,
> +                                new_key ? new_key : entry->key);
>               delete = true;
>           } else if (prefix) {
>               /* All other objects are moved to the target unchanged. */
> @@ -531,7 +568,7 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
>    */
>   void qdict_flatten(QDict *qdict)
>   {
> -    qdict_do_flatten(qdict, qdict, NULL);
> +    qdict_flatten_qdict(qdict, qdict, NULL);

If there was an assumption that the top level is a qdict, also assert 
that here?

Thanks,

Fam