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

[Eric Blake]

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 <address@hidden>
> ---
>   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?

> + */
> +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 <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org