Re: [Qemu-devel] [PATCH 07/13] qdict: Add qdict_stringify_for_keyval()

From: Max Reitz <mreitz@redhat.com>
To: Eric Blake <eblake@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	Kevin Wolf <kwolf@redhat.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 07/13] qdict: Add qdict_stringify_for_keyval()
Date: Fri, 11 May 2018 23:42:33 +0200	[thread overview]
Message-ID: <51b724fd-ed60-1021-cf26-729e6a0f7590@redhat.com> (raw)
In-Reply-To: <e0cb86f1-ac04-f7cd-bd19-77abeeb5636a@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]

On 2018-05-11 20:39, Eric Blake wrote:
> On 05/09/2018 11:55 AM, Max Reitz wrote:
>> The purpose of this function is to prepare a QDict for consumption by
>> the keyval visitor, which only accepts strings and QNull.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   include/block/qdict.h |  2 ++
>>   qobject/qdict.c       | 57
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 59 insertions(+)
>>
> 
>> +/**
>> + * Convert all values in a QDict so it can be consumed by the keyval
>> + * input visitor.  QNull values are left as-is, all other values are
>> + * converted to strings.
>> + *
>> + * @qdict must be flattened, i.e. it may not contain any nested QDicts
>> + * or QLists.
> 
> Maybe s/flattened/flattened already/
> 
> or would it be worth letting this function PERFORM the flattening step
> automatically?

Possibly, but I don't think it would be any more efficient, so I'd
rather leave it up to the caller to do it explicitly than to do it here
and maybe surprise someone.

(Indeed I personally prefer to be surprised by an abort() than by some
unintended data change.)

Max

>> + */
>> +void qdict_stringify_for_keyval(QDict *qdict)
>> +{
>> +    const QDictEntry *e;
>> +
>> +    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
>> +        QString *new_value = NULL;
>> +
>> +        switch (qobject_type(e->value)) {
>> +        case QTYPE_QNULL:
>> +            /* leave as-is */
>> +            break;
>> +
>> +        case QTYPE_QNUM: {
>> +            char *str = qnum_to_string(qobject_to(QNum, e->value));
>> +            new_value = qstring_from_str(str);
>> +            g_free(str);
>> +            break;
>> +        }
>> +
>> +        case QTYPE_QSTRING:
>> +            /* leave as-is */
>> +            break;
>> +
>> +        case QTYPE_QDICT:
>> +        case QTYPE_QLIST:
>> +            /* @qdict must be flattened */
>> +            abort();
> 
> Matches your documentation about requiring it to be already flattened.
> 
>> +
>> +        case QTYPE_QBOOL:
>> +            if (qbool_get_bool(qobject_to(QBool, e->value))) {
>> +                new_value = qstring_from_str("on");
>> +            } else {
>> +                new_value = qstring_from_str("off");
>> +            }
>> +            break;
>> +
>> +        case QTYPE_NONE:
>> +        case QTYPE__MAX:
>> +            abort();
>> +        }
>> +
>> +        if (new_value) {
>> +            QDictEntry *mut_e = (QDictEntry *)e;
> 
> A bit of a shame that you have to cast away const. It took me a couple
> reads to figure out this meant 'mutable_e'.  But in the end, the code
> looks correct.
> 
>> +            qobject_unref(mut_e->value);
>> +            mut_e->value = QOBJECT(new_value);
> 
> If we're okay requiring the caller to pre-flatten before calling this,
> instead of offering to do it here,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]