Re: [Qemu-block] [PATCH v2 11/36] qdict: Introduce qdict_rename_keys()

[Eric Blake]

On 02/21/2018 07:53 AM, Kevin Wolf wrote:
> A few block drivers will need to rename .bdrv_create options for their
> QAPIfication, so let's have a helper function for that.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   include/qapi/qmp/qdict.h |   6 +++
>   qobject/qdict.c          |  34 ++++++++++++++
>   tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)

> +/**
> + * qdict_rename_keys(): Rename keys in qdict according to the replacements
> + * specified in the array renames. The array must be terminated by an entry
> + * with from = NULL.
> + *
> + * The renames are performed individually in the order of the array, so entries
> + * may be renamed multiple times and may or may not conflict depending on the
> + * order of the renames array.

Oh interesting - so I could rename a->tmp, b->a, tmp->b in the classic 
strategy to intentionally avoid conflicts.  But I hope none of our 
actual clients ever abuse the interface that directly.

> + *
> + * Returns true for success, false in error cases.

I won't make you change it, but is 0/-1 any easier to understand 
intuitively?  With bool, it's often a case of "I'd better check the docs 
for whether true meant the sense I wanted"

> + */
> +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
> +{

> +++ b/tests/check-qdict.c

> +    copy = qdict_clone_shallow(dict);
> +    qdict_rename_keys(copy, renames, &error_abort);
> +
> +    g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo");
> +    g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar");
> +    g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42);
> +    g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true);
> +    g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL);

Also worth an assert that there are exactly 5 keys, so that rename 
didn't botch something to leave a different straggler key behind?

> +
> +    QDECREF(copy);
> +
> +    /* Simple rename of all entries */
> +    renames = (QDictRenames[]) {
> +        { "abc",        "str1" },
> +        { "abcdef",     "str2" },
> +        { "number",     "int" },
> +        { "flag",       "bool" },
> +        { "nothing",    "null" },
> +        { NULL , NULL }
> +    };
> +    copy = qdict_clone_shallow(dict);
> +    qdict_rename_keys(copy, renames, &error_abort);
> +
> +    g_assert(!qdict_haskey(copy, "abc"));
> +    g_assert(!qdict_haskey(copy, "abcdef"));
> +    g_assert(!qdict_haskey(copy, "number"));
> +    g_assert(!qdict_haskey(copy, "flag"));
> +    g_assert(!qdict_haskey(copy, "nothing"));

Direct check for the obvious keys that should have been renamed,

> +
> +    g_assert_cmpstr(qdict_get_str(copy, "str1"), ==, "foo");
> +    g_assert_cmpstr(qdict_get_str(copy, "str2"), ==, "bar");
> +    g_assert_cmpint(qdict_get_int(copy, "int"), ==, 42);
> +    g_assert_cmpint(qdict_get_bool(copy, "bool"), ==, true);
> +    g_assert(qobject_type(qdict_get(copy, "null")) == QTYPE_QNULL);

but again, an assert that there are 5 keys rules out all other mistakes, 
too.

Up to you whether to further tweak the tests; and with the spelling fix 
Max already found,

Reviewed-by: Eric Blake <address@hidden>

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