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

[Max Reitz <mreitz@redhat.com>]

On 12.12.2013 07:17, Fam Zheng wrote:
> 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?

Yes, I'll do that.

>
>> +
>> + 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?

I think I rather forgot to support other types. Those should be moved to 
the target QDict, just as qdict_flatten_qdict() does it.

>
>> + 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?

Well, the type is already QDict *, therefore I think we are safe to 
assume it is indeed one – if that is what you meant. ;-)

The question to me would be whether we should be supporting QLists here, 
too. Since this would make qdict_flatten_qlist() more complex, I think 
we should not do that until it is required.

Thank you,

Max