diff mbox series

[v2,11/36] qdict: Introduce qdict_rename_keys()

Message ID 20180221135404.27598-12-kwolf@redhat.com
State New
Headers show
Series x-blockdev-create for protocols and qcow2 | expand

Commit Message

Kevin Wolf Feb. 21, 2018, 1:53 p.m. UTC
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 <kwolf@redhat.com>
---
 include/qapi/qmp/qdict.h |   6 +++
 qobject/qdict.c          |  34 ++++++++++++++
 tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)

Comments

Max Reitz Feb. 22, 2018, 10:40 p.m. UTC | #1
On 2018-02-21 14:53, 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 <kwolf@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |   6 +++
>  qobject/qdict.c          |  34 ++++++++++++++
>  tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+)

[...]

> diff --git a/tests/check-qdict.c b/tests/check-qdict.c
> index ec628f3453..5f8f3be9ff 100644
> --- a/tests/check-qdict.c
> +++ b/tests/check-qdict.c
> @@ -665,6 +665,117 @@ static void qdict_crumple_test_empty(void)

[...]

> +    /* Renames are processed top to bottom */
> +    renames = (QDictRenames[]) {
> +        { "abc",        "tmp" },
> +        { "abcdef",     "abc" },
> +        { "number",     "abcdef" },
> +        { "flag",       "number" },
> +        { "nothing",    "flag" },
> +        { "tmp",        "nothing" },
> +        { NULL , NULL }
> +    };

A bit confusing to follow, but I guess nobody will have to follow it
after me and Eric.

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

*rename

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>
Eric Blake Feb. 22, 2018, 11:13 p.m. UTC | #2
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 <kwolf@redhat.com>
> ---
>   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 <eblake@redhat.com>
diff mbox series

Patch

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index ff6f7842c3..7c6d844549 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -81,4 +81,10 @@  QObject *qdict_crumple(const QDict *src, Error **errp);
 
 void qdict_join(QDict *dest, QDict *src, bool overwrite);
 
+typedef struct QDictRenames {
+    const char *from;
+    const char *to;
+} QDictRenames;
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp);
+
 #endif /* QDICT_H */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 23df84f9cd..229b8c840b 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -1072,3 +1072,37 @@  void qdict_join(QDict *dest, QDict *src, bool overwrite)
         entry = next;
     }
 }
+
+/**
+ * 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.
+ *
+ * Returns true for success, false in error cases.
+ */
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
+{
+    QObject *qobj;
+
+    while (renames->from) {
+        if (qdict_haskey(qdict, renames->from)) {
+            if (qdict_haskey(qdict, renames->to)) {
+                error_setg(errp, "'%s' and its alias '%s' can't be used at the "
+                           "same time", renames->to, renames->from);
+                return false;
+            }
+
+            qobj = qdict_get(qdict, renames->from);
+            qobject_incref(qobj);
+            qdict_put_obj(qdict, renames->to, qobj);
+            qdict_del(qdict, renames->from);
+        }
+
+        renames++;
+    }
+    return true;
+}
diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index ec628f3453..5f8f3be9ff 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -665,6 +665,117 @@  static void qdict_crumple_test_empty(void)
     QDECREF(dst);
 }
 
+static void qdict_rename_keys_test(void)
+{
+    QDict *dict = qdict_new();
+    QDict *copy;
+    QDictRenames *renames;
+    Error *local_err = NULL;
+
+    qdict_put_str(dict, "abc", "foo");
+    qdict_put_str(dict, "abcdef", "bar");
+    qdict_put_int(dict, "number", 42);
+    qdict_put_bool(dict, "flag", true);
+    qdict_put_null(dict, "nothing");
+
+    /* Empty rename list */
+    renames = (QDictRenames[]) {
+        { NULL, "this can be anything" }
+    };
+    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);
+
+    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"));
+
+    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);
+
+    QDECREF(copy);
+
+    /* Renames are processed top to bottom */
+    renames = (QDictRenames[]) {
+        { "abc",        "tmp" },
+        { "abcdef",     "abc" },
+        { "number",     "abcdef" },
+        { "flag",       "number" },
+        { "nothing",    "flag" },
+        { "tmp",        "nothing" },
+        { NULL , NULL }
+    };
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &error_abort);
+
+    g_assert_cmpstr(qdict_get_str(copy, "nothing"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "bar");
+    g_assert_cmpint(qdict_get_int(copy, "abcdef"), ==, 42);
+    g_assert_cmpint(qdict_get_bool(copy, "number"), ==, true);
+    g_assert(qobject_type(qdict_get(copy, "flag")) == QTYPE_QNULL);
+    g_assert(!qdict_haskey(copy, "tmp"));
+
+    QDECREF(copy);
+
+    /* Conflicting renam */
+    renames = (QDictRenames[]) {
+        { "abcdef",     "abc" },
+        { NULL , NULL }
+    };
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &local_err);
+
+    g_assert(local_err != NULL);
+    error_free(local_err);
+    local_err = NULL;
+
+    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);
+
+    QDECREF(copy);
+
+    /* Renames in an empty dict */
+    renames = (QDictRenames[]) {
+        { "abcdef",     "abc" },
+        { NULL , NULL }
+    };
+
+    QDECREF(dict);
+    dict = qdict_new();
+
+    qdict_rename_keys(dict, renames, &error_abort);
+    g_assert(qdict_first(dict) == NULL);
+
+    QDECREF(dict);
+}
+
 static void qdict_crumple_test_bad_inputs(void)
 {
     QDict *src;
@@ -880,6 +991,8 @@  int main(int argc, char **argv)
     g_test_add_func("/public/crumple/bad_inputs",
                     qdict_crumple_test_bad_inputs);
 
+    g_test_add_func("/public/rename_keys", qdict_rename_keys_test);
+
     /* The Big one */
     if (g_test_slow()) {
         g_test_add_func("/stress/test", qdict_stress_test);