[Qemu-devel] [PATCH v5 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Qemu-devel] [PATCH v5 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Max Reitz]

bdrv_reopen_prepare() assumes that all BDS options are strings, which is
not necessarily correct. This series introduces a new qobject_is_equal()
function which can be used to test whether any options have changed,
independently of their type.


v5:
- Patch 1: Changes due to rebase
- Patch 2: Added
- Patch 3: Made double QNums always unequal to integer QNums [Markus]
- Patch 6:
  - Removed many tests because they no longer make sense with double
    QNums generally being unequal to integer QNums
  - Use NAN from math.h instead of 0.0 / 0.0 [Eric]
  - dm42 should be a double, not an int (and I didn't notice because
    -42.0 was supposed to be equal to -42 anyway)
  - Rebase changes because QNull is now an own type


git-backport-diff against v4:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[0023] [FC] 'qapi/qnull: Add own header'
002/6:[down] 'qapi/qlist: Add qlist_append_null() macro'
003/6:[0035] [FC] 'qapi: Add qobject_is_equal()'
004/6:[----] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
005/6:[----] [--] 'iotests: Add test for non-string option reopening'
006/6:[0153] [FC] 'tests: Add check-qobject for equality tests'


Max Reitz (6):
  qapi/qnull: Add own header
  qapi/qlist: Add qlist_append_null() macro
  qapi: Add qobject_is_equal()
  block: qobject_is_equal() in bdrv_reopen_prepare()
  iotests: Add test for non-string option reopening
  tests: Add check-qobject for equality tests

 tests/Makefile.include      |   4 +-
 include/qapi/qmp/qbool.h    |   1 +
 include/qapi/qmp/qdict.h    |   2 +
 include/qapi/qmp/qlist.h    |   4 +
 include/qapi/qmp/qnull.h    |  32 +++++
 include/qapi/qmp/qnum.h     |   1 +
 include/qapi/qmp/qobject.h  |  21 ++-
 include/qapi/qmp/qstring.h  |   1 +
 include/qapi/qmp/types.h    |   1 +
 block.c                     |  29 ++--
 qapi/qapi-clone-visitor.c   |   1 +
 qapi/string-input-visitor.c |   1 +
 qobject/qbool.c             |   8 ++
 qobject/qdict.c             |  29 ++++
 qobject/qlist.c             |  32 +++++
 qobject/qnull.c             |  11 +-
 qobject/qnum.c              |  54 ++++++++
 qobject/qobject.c           |  29 ++++
 qobject/qstring.c           |   9 ++
 tests/check-qnull.c         |   2 +-
 tests/check-qobject.c       | 315 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/133      |   9 ++
 tests/qemu-iotests/133.out  |   5 +
 23 files changed, 575 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.13.5

[Qemu-devel] [PATCH v5 1/6] qapi/qnull: Add own header

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qdict.h    |  1 +
 include/qapi/qmp/qnull.h    | 30 ++++++++++++++++++++++++++++++
 include/qapi/qmp/qobject.h  | 12 ------------
 include/qapi/qmp/types.h    |  1 +
 qapi/qapi-clone-visitor.c   |  1 +
 qapi/string-input-visitor.c |  1 +
 qobject/qnull.c             |  2 +-
 tests/check-qnull.c         |  2 +-
 8 files changed, 36 insertions(+), 14 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 6588c7f0c8..7ea5120c4a 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qnum.h"
 #include "qemu/queue.h"
 
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
new file mode 100644
index 0000000000..d075549283
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,30 @@
+/*
+ * QNull
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later.  See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef QNULL_H
+#define QNULL_H
+
+#include "qapi/qmp/qobject.h"
+
+struct QNull {
+    QObject base;
+};
+
+extern QNull qnull_;
+
+static inline QNull *qnull(void)
+{
+    QINCREF(&qnull_);
+    return &qnull_;
+}
+
+#endif /* QNULL_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index eab29edd12..ef1d1a9237 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,16 +93,4 @@ static inline QType qobject_type(const QObject *obj)
     return obj->type;
 }
 
-struct QNull {
-    QObject base;
-};
-
-extern QNull qnull_;
-
-static inline QNull *qnull(void)
-{
-    QINCREF(&qnull_);
-    return &qnull_;
-}
-
 #endif /* QOBJECT_H */
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index a4bc662bfb..749ac44dcb 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -19,5 +19,6 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 
 #endif /* QAPI_QMP_TYPES_H */
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index d8b62792bc..daab6819b4 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -12,6 +12,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qnull.h"
 
 struct QapiCloneVisitor {
     Visitor visitor;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 67a0a4a58b..b3fdd0827d 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -16,6 +16,7 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
diff --git a/qobject/qnull.c b/qobject/qnull.c
index 69a21d1059..bc9fd31626 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 
 QNull qnull_ = {
     .base = {
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 5c6eb0adc8..afa4400da1 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -8,7 +8,7 @@
  */
 #include "qemu/osdep.h"
 
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu-common.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
-- 
2.13.5

[Qemu-devel] [PATCH v5 2/6] qapi/qlist: Add qlist_append_null() macro

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qlist.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index c4b5fdad9b..59d209bbae 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/queue.h"
 
 typedef struct QListEntry {
@@ -37,6 +38,8 @@ typedef struct QList {
         qlist_append(qlist, qbool_from_bool(value))
 #define qlist_append_str(qlist, value) \
         qlist_append(qlist, qstring_from_str(value))
+#define qlist_append_null(qlist) \
+        qlist_append(qlist, qnull())
 
 #define QLIST_FOREACH_ENTRY(qlist, var)             \
         for ((var) = ((qlist)->head.tqh_first);     \
-- 
2.13.5

[Qemu-devel] [PATCH v5 3/6] qapi: Add qobject_is_equal()

[Max Reitz]

This generic function (along with its implementations for different
types) determines whether two QObjects are equal.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   |  2 ++
 include/qapi/qmp/qnum.h    |  1 +
 include/qapi/qmp/qobject.h |  9 ++++++++
 include/qapi/qmp/qstring.h |  1 +
 qobject/qbool.c            |  8 +++++++
 qobject/qdict.c            | 29 +++++++++++++++++++++++++
 qobject/qlist.c            | 32 +++++++++++++++++++++++++++
 qobject/qnull.c            |  9 ++++++++
 qobject/qnum.c             | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 qobject/qobject.c          | 29 +++++++++++++++++++++++++
 qobject/qstring.c          |  9 ++++++++
 14 files changed, 186 insertions(+)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index a41111c309..f77ea86c4e 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -24,6 +24,7 @@ typedef struct QBool {
 QBool *qbool_from_bool(bool value);
 bool qbool_get_bool(const QBool *qb);
 QBool *qobject_to_qbool(const QObject *obj);
+bool qbool_is_equal(const QObject *x, const QObject *y);
 void qbool_destroy_obj(QObject *obj);
 
 #endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 7ea5120c4a..fc218e7be6 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -43,6 +43,7 @@ void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
+bool qdict_is_equal(const QObject *x, const QObject *y);
 void qdict_iter(const QDict *qdict,
                 void (*iter)(const char *key, QObject *obj, void *opaque),
                 void *opaque);
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 59d209bbae..ec3fcc1a4c 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -61,6 +61,7 @@ QObject *qlist_peek(QList *qlist);
 int qlist_empty(const QList *qlist);
 size_t qlist_size(const QList *qlist);
 QList *qobject_to_qlist(const QObject *obj);
+bool qlist_is_equal(const QObject *x, const QObject *y);
 void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index d075549283..c992ee2ae1 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -27,4 +27,6 @@ static inline QNull *qnull(void)
     return &qnull_;
 }
 
+bool qnull_is_equal(const QObject *x, const QObject *y);
+
 #endif /* QNULL_H */
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index d6b0791139..c3d86794bb 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -69,6 +69,7 @@ double qnum_get_double(QNum *qn);
 char *qnum_to_string(QNum *qn);
 
 QNum *qobject_to_qnum(const QObject *obj);
+bool qnum_is_equal(const QObject *x, const QObject *y);
 void qnum_destroy_obj(QObject *obj);
 
 #endif /* QNUM_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index ef1d1a9237..38ac68845c 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -68,6 +68,15 @@ static inline void qobject_incref(QObject *obj)
 }
 
 /**
+ * qobject_is_equal(): Return whether the two objects are equal.
+ *
+ * Any of the pointers may be NULL; return true if both are.  Always
+ * return false if only one is (therefore a QNull object is not
+ * considered equal to a NULL pointer).
+ */
+bool qobject_is_equal(const QObject *x, const QObject *y);
+
+/**
  * qobject_destroy(): Free resources used by the object
  */
 void qobject_destroy(QObject *obj);
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 10076b7c8c..65c05a9be5 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -31,6 +31,7 @@ void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 QString *qobject_to_qstring(const QObject *obj);
+bool qstring_is_equal(const QObject *x, const QObject *y);
 void qstring_destroy_obj(QObject *obj);
 
 #endif /* QSTRING_H */
diff --git a/qobject/qbool.c b/qobject/qbool.c
index 0606bbd2a3..ac825fc5a2 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
 }
 
 /**
+ * qbool_is_equal(): Test whether the two QBools are equal
+ */
+bool qbool_is_equal(const QObject *x, const QObject *y)
+{
+    return qobject_to_qbool(x)->value == qobject_to_qbool(y)->value;
+}
+
+/**
  * qbool_destroy_obj(): Free all memory allocated by a
  * QBool object
  */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 576018e531..e8f15f1132 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -403,6 +403,35 @@ void qdict_del(QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_is_equal(): Test whether the two QDicts are equal
+ *
+ * Here, equality means whether they contain the same keys and whether
+ * the respective values are in turn equal (i.e. invoking
+ * qobject_is_equal() on them yields true).
+ */
+bool qdict_is_equal(const QObject *x, const QObject *y)
+{
+    const QDict *dict_x = qobject_to_qdict(x);
+    const QDict *dict_y = qobject_to_qdict(y);
+    const QDictEntry *e;
+
+    if (qdict_size(dict_x) != qdict_size(dict_y)) {
+        return false;
+    }
+
+    for (e = qdict_first(dict_x); e; e = qdict_next(dict_x, e)) {
+        const QObject *obj_x = qdict_entry_value(e);
+        const QObject *obj_y = qdict_get(dict_y, qdict_entry_key(e));
+
+        if (!qobject_is_equal(obj_x, obj_y)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+/**
  * qdict_destroy_obj(): Free all the memory allocated by a QDict
  */
 void qdict_destroy_obj(QObject *obj)
diff --git a/qobject/qlist.c b/qobject/qlist.c
index 86b60cb88c..3ef57d31d1 100644
--- a/qobject/qlist.c
+++ b/qobject/qlist.c
@@ -140,6 +140,38 @@ QList *qobject_to_qlist(const QObject *obj)
 }
 
 /**
+ * qlist_is_equal(): Test whether the two QLists are equal
+ *
+ * In order to be considered equal, the respective two objects at each
+ * index of the two lists have to compare equal (regarding
+ * qobject_is_equal()), and both lists have to have the same number of
+ * elements.
+ * That means both lists have to contain equal objects in equal order.
+ */
+bool qlist_is_equal(const QObject *x, const QObject *y)
+{
+    const QList *list_x = qobject_to_qlist(x);
+    const QList *list_y = qobject_to_qlist(y);
+    const QListEntry *entry_x, *entry_y;
+
+    entry_x = qlist_first(list_x);
+    entry_y = qlist_first(list_y);
+
+    while (entry_x && entry_y) {
+        if (!qobject_is_equal(qlist_entry_obj(entry_x),
+                              qlist_entry_obj(entry_y)))
+        {
+            return false;
+        }
+
+        entry_x = qlist_next(entry_x);
+        entry_y = qlist_next(entry_y);
+    }
+
+    return !entry_x && !entry_y;
+}
+
+/**
  * qlist_destroy_obj(): Free all the memory allocated by a QList
  */
 void qlist_destroy_obj(QObject *obj)
diff --git a/qobject/qnull.c b/qobject/qnull.c
index bc9fd31626..f6f55f11ea 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -20,3 +20,12 @@ QNull qnull_ = {
         .refcnt = 1,
     },
 };
+
+/**
+ * qnull_is_equal(): Always return true because any two QNull objects
+ * are equal.
+ */
+bool qnull_is_equal(const QObject *x, const QObject *y)
+{
+    return true;
+}
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 476e81c93b..410686a611 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -213,6 +213,60 @@ QNum *qobject_to_qnum(const QObject *obj)
 }
 
 /**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ *
+ * Negative integers are never considered equal to unsigned integers,
+ * but positive integers in the range [0, INT64_MAX] are considered
+ * equal independently of whether the QNum's kind is i64 or u64.
+ *
+ * Doubles are never considered equal to integers.
+ */
+bool qnum_is_equal(const QObject *x, const QObject *y)
+{
+    QNum *num_x = qobject_to_qnum(x);
+    QNum *num_y = qobject_to_qnum(y);
+
+    switch (num_x->kind) {
+    case QNUM_I64:
+        switch (num_y->kind) {
+        case QNUM_I64:
+            /* Comparison in native int64_t type */
+            return num_x->u.i64 == num_y->u.i64;
+        case QNUM_U64:
+            /* Implicit conversion of x to uin64_t, so we have to
+             * check its sign before */
+            return num_x->u.i64 >= 0 && num_x->u.i64 == num_y->u.u64;
+        case QNUM_DOUBLE:
+            return false;
+        }
+        abort();
+    case QNUM_U64:
+        switch (num_y->kind) {
+        case QNUM_I64:
+            return qnum_is_equal(y, x);
+        case QNUM_U64:
+            /* Comparison in native uint64_t type */
+            return num_x->u.u64 == num_y->u.u64;
+        case QNUM_DOUBLE:
+            return false;
+        }
+        abort();
+    case QNUM_DOUBLE:
+        switch (num_y->kind) {
+        case QNUM_I64:
+        case QNUM_U64:
+            return false;
+        case QNUM_DOUBLE:
+            /* Comparison in native double type */
+            return num_x->u.dbl == num_y->u.dbl;
+        }
+        abort();
+    }
+
+    abort();
+}
+
+/**
  * qnum_destroy_obj(): Free all memory allocated by a
  * QNum object
  */
diff --git a/qobject/qobject.c b/qobject/qobject.c
index b0cafb66f1..b2a536041d 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -27,3 +27,32 @@ void qobject_destroy(QObject *obj)
     assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
     qdestroy[obj->type](obj);
 }
+
+
+static bool (*qis_equal[QTYPE__MAX])(const QObject *, const QObject *) = {
+    [QTYPE_NONE] = NULL,               /* No such object exists */
+    [QTYPE_QNULL] = qnull_is_equal,
+    [QTYPE_QNUM] = qnum_is_equal,
+    [QTYPE_QSTRING] = qstring_is_equal,
+    [QTYPE_QDICT] = qdict_is_equal,
+    [QTYPE_QLIST] = qlist_is_equal,
+    [QTYPE_QBOOL] = qbool_is_equal,
+};
+
+bool qobject_is_equal(const QObject *x, const QObject *y)
+{
+    /* We cannot test x == y because an object does not need to be
+     * equal to itself (e.g. NaN floats are not). */
+
+    if (!x && !y) {
+        return true;
+    }
+
+    if (!x || !y || x->type != y->type) {
+        return false;
+    }
+
+    assert(QTYPE_NONE < x->type && x->type < QTYPE__MAX);
+
+    return qis_equal[x->type](x, y);
+}
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 5da7b5f37c..74182a1c02 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -129,6 +129,15 @@ const char *qstring_get_str(const QString *qstring)
 }
 
 /**
+ * qstring_is_equal(): Test whether the two QStrings are equal
+ */
+bool qstring_is_equal(const QObject *x, const QObject *y)
+{
+    return !strcmp(qobject_to_qstring(x)->string,
+                   qobject_to_qstring(y)->string);
+}
+
+/**
  * qstring_destroy_obj(): Free all memory allocated by a QString
  * object
  */
-- 
2.13.5

[Qemu-devel] [PATCH v5 4/6] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

Currently, bdrv_reopen_prepare() assumes that all BDS options are
strings. However, this is not the case if the BDS has been created
through the json: pseudo-protocol or blockdev-add.

Note that the user-invokable reopen command is an HMP command, so you
can only specify strings there. Therefore, specifying a non-string
option with the "same" value as it was when originally created will now
return an error because the values are supposedly similar (and there is
no way for the user to circumvent this but to just not specify the
option again -- however, this is still strictly better than just
crashing).

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 6dd47e414e..0c16f4dff1 100644
--- a/block.c
+++ b/block.c
@@ -2956,19 +2956,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         const QDictEntry *entry = qdict_first(reopen_state->options);
 
         do {
-            QString *new_obj = qobject_to_qstring(entry->value);
-            const char *new = qstring_get_str(new_obj);
+            QObject *new = entry->value;
+            QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+
             /*
-             * Caution: while qdict_get_try_str() is fine, getting
-             * non-string types would require more care.  When
-             * bs->options come from -blockdev or blockdev_add, its
-             * members are typed according to the QAPI schema, but
-             * when they come from -drive, they're all QString.
+             * TODO: When using -drive to specify blockdev options, all values
+             * will be strings; however, when using -blockdev, blockdev-add or
+             * filenames using the json:{} pseudo-protocol, they will be
+             * correctly typed.
+             * In contrast, reopening options are (currently) always strings
+             * (because you can only specify them through qemu-io; all other
+             * callers do not specify any options).
+             * Therefore, when using anything other than -drive to create a BDS,
+             * this cannot detect non-string options as unchanged, because
+             * qobject_is_equal() always returns false for objects of different
+             * type.  In the future, this should be remedied by correctly typing
+             * all options.  For now, this is not too big of an issue because
+             * the user can simply omit options which cannot be changed anyway,
+             * so they will stay unchanged.
              */
-            const char *old = qdict_get_try_str(reopen_state->bs->options,
-                                                entry->key);
-
-            if (!old || strcmp(new, old)) {
+            if (!qobject_is_equal(new, old)) {
                 error_setg(errp, "Cannot change the option '%s'", entry->key);
                 ret = -EINVAL;
                 goto error;
-- 
2.13.5

[Qemu-devel] [PATCH v5 5/6] iotests: Add test for non-string option reopening

[Max Reitz]

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/133     | 9 +++++++++
 tests/qemu-iotests/133.out | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 9d35a6a1ca..af6b3e1dd4 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -83,6 +83,15 @@ $QEMU_IO -c 'reopen -o driver=qcow2' $TEST_IMG
 $QEMU_IO -c 'reopen -o file.driver=file' $TEST_IMG
 $QEMU_IO -c 'reopen -o backing.driver=qcow2' $TEST_IMG
 
+echo
+echo "=== Check that reopening works with non-string options ==="
+echo
+
+# Using the json: pseudo-protocol we can create non-string options
+# (Invoke 'info' just so we get some output afterwards)
+IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
+    "json:{'driver': 'null-co', 'size': 65536}"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index cc86b94880..f4a85aeb63 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -19,4 +19,9 @@ Cannot change the option 'driver'
 
 === Check that unchanged driver is okay ===
 
+
+=== Check that reopening works with non-string options ===
+
+format name: null-co
+format name: null-co
 *** done
-- 
2.13.5

[Qemu-devel] [PATCH v5 6/6] tests: Add check-qobject for equality tests

[Max Reitz]

Add a new test file (check-qobject.c) for unit tests that concern
QObjects as a whole.

Its only purpose for now is to test the qobject_is_equal() function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/Makefile.include |   4 +-
 tests/check-qobject.c  | 315 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 318 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qobject.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index fae5715e9c..40b8b4e98f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -41,6 +41,7 @@ check-unit-y += tests/check-qlist$(EXESUF)
 gcov-files-check-qlist-y = qobject/qlist.c
 check-unit-y += tests/check-qnull$(EXESUF)
 gcov-files-check-qnull-y = qobject/qnull.c
+check-unit-y += tests/check-qobject$(EXESUF)
 check-unit-y += tests/check-qjson$(EXESUF)
 gcov-files-check-qjson-y = qobject/qjson.c
 check-unit-y += tests/check-qlit$(EXESUF)
@@ -542,7 +543,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \
 	tests/test-qmp-introspect.h
 
 test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
-	tests/check-qlist.o tests/check-qnull.o \
+	tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
 	tests/check-qjson.o tests/check-qlit.o \
 	tests/test-coroutine.o tests/test-string-output-visitor.o \
 	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
@@ -576,6 +577,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
+tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
 tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
new file mode 100644
index 0000000000..8f1b5550c2
--- /dev/null
+++ b/tests/check-qobject.c
@@ -0,0 +1,315 @@
+/*
+ * Generic QObject unit-tests.
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+
+#include "qapi/qmp/types.h"
+#include "qemu-common.h"
+
+#include <math.h>
+
+/* Marks the end of the test_equality() argument list.
+ * We cannot use NULL there because that is a valid argument. */
+static QObject _test_equality_end_of_arguments;
+
+/**
+ * Test whether all variadic QObject *arguments are equal (@expected
+ * is true) or whether they are all not equal (@expected is false).
+ * Every QObject is tested to be equal to itself (to test
+ * reflexivity), all tests are done both ways (to test symmetry), and
+ * transitivity is not assumed but checked (each object is compared to
+ * every other one).
+ *
+ * Note that qobject_is_equal() is not really an equivalence relation,
+ * so this function may not be used for all objects (reflexivity is
+ * not guaranteed, e.g. in the case of a QNum containing NaN).
+ */
+static void do_test_equality(bool expected, ...)
+{
+    va_list ap_count, ap_extract;
+    QObject **args;
+    int arg_count = 0;
+    int i, j;
+
+    va_start(ap_count, expected);
+    va_copy(ap_extract, ap_count);
+    while (va_arg(ap_count, QObject *) != &_test_equality_end_of_arguments) {
+        arg_count++;
+    }
+    va_end(ap_count);
+
+    args = g_new(QObject *, arg_count);
+    for (i = 0; i < arg_count; i++) {
+        args[i] = va_arg(ap_extract, QObject *);
+    }
+    va_end(ap_extract);
+
+    for (i = 0; i < arg_count; i++) {
+        g_assert(qobject_is_equal(args[i], args[i]) == true);
+
+        for (j = i + 1; j < arg_count; j++) {
+            g_assert(qobject_is_equal(args[i], args[j]) == expected);
+        }
+    }
+}
+
+#define test_equality(expected, ...) \
+    do_test_equality(expected, __VA_ARGS__, &_test_equality_end_of_arguments)
+
+static void do_free_all(int _, ...)
+{
+    va_list ap;
+    QObject *obj;
+
+    va_start(ap, _);
+    while ((obj = va_arg(ap, QObject *)) != NULL) {
+        qobject_decref(obj);
+    }
+    va_end(ap);
+}
+
+#define free_all(...) \
+    do_free_all(0, __VA_ARGS__, NULL)
+
+static void qobject_is_equal_null_test(void)
+{
+    test_equality(false, qnull(), NULL);
+}
+
+static void qobject_is_equal_num_test(void)
+{
+    QNum *u0, *i0, *d0, *dnan, *um42, *im42, *dm42;
+    QString *s0, *s_empty;
+    QBool *bfalse;
+
+    u0 = qnum_from_uint(0u);
+    i0 = qnum_from_int(0);
+    d0 = qnum_from_double(0.0);
+    dnan = qnum_from_double(NAN);
+    um42 = qnum_from_uint((uint64_t)-42);
+    im42 = qnum_from_int(-42);
+    dm42 = qnum_from_double(-42.0);
+
+    s0 = qstring_from_str("0");
+    s_empty = qstring_new();
+    bfalse = qbool_from_bool(false);
+
+    /* Integers representing a mathematically equal number should
+     * compare equal */
+    test_equality(true, u0, i0);
+    /* Doubles, however, are always unequal to integers */
+    test_equality(false, u0, d0);
+    test_equality(false, i0, d0);
+
+    /* No automatic type conversion */
+    test_equality(false, u0, s0, s_empty, bfalse, qnull(), NULL);
+    test_equality(false, i0, s0, s_empty, bfalse, qnull(), NULL);
+    test_equality(false, d0, s0, s_empty, bfalse, qnull(), NULL);
+
+    /* Do not assume any object is equal to itself -- note however
+     * that NaN cannot occur in a JSON object anyway. */
+    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
+
+    /* No unsigned overflow */
+    test_equality(false, um42, im42);
+    test_equality(false, um42, dm42);
+    g_assert(qobject_is_equal(QOBJECT(im42), QOBJECT(dm42)) == false);
+    test_equality(false, im42, dm42);
+
+
+    free_all(u0, i0, d0, dnan, um42, im42, dm42, s0, s_empty, bfalse);
+}
+
+static void qobject_is_equal_bool_test(void)
+{
+    QBool *btrue_0, *btrue_1, *bfalse_0, *bfalse_1;
+
+    /* Automatic type conversion is tested in the QNum test */
+
+    btrue_0 = qbool_from_bool(true);
+    btrue_1 = qbool_from_bool(true);
+    bfalse_0 = qbool_from_bool(false);
+    bfalse_1 = qbool_from_bool(false);
+
+    test_equality(true, btrue_0, btrue_1);
+    test_equality(true, bfalse_0, bfalse_1);
+    test_equality(false, btrue_0, bfalse_0);
+    test_equality(false, btrue_1, bfalse_1);
+
+    free_all(btrue_0, btrue_1, bfalse_0, bfalse_1);
+}
+
+static void qobject_is_equal_string_test(void)
+{
+    QString *str_base, *str_whitespace_0, *str_whitespace_1, *str_whitespace_2;
+    QString *str_whitespace_3, *str_case, *str_built;
+
+    str_base = qstring_from_str("foo");
+    str_whitespace_0 = qstring_from_str(" foo");
+    str_whitespace_1 = qstring_from_str("foo ");
+    str_whitespace_2 = qstring_from_str("foo\b");
+    str_whitespace_3 = qstring_from_str("fooo\b");
+    str_case = qstring_from_str("Foo");
+
+    /* Should yield "foo" */
+    str_built = qstring_from_substr("form", 0, 1);
+    qstring_append_chr(str_built, 'o');
+
+    test_equality(false, str_base, str_whitespace_0, str_whitespace_1,
+                         str_whitespace_2, str_whitespace_3, str_case);
+
+    test_equality(true, str_base, str_built);
+
+    free_all(str_base, str_whitespace_0, str_whitespace_1, str_whitespace_2,
+             str_whitespace_3, str_case, str_built);
+}
+
+static void qobject_is_equal_list_test(void)
+{
+    QList *list_0, *list_1, *list_cloned;
+    QList *list_reordered, *list_longer, *list_shorter;
+
+    list_0 = qlist_new();
+    list_1 = qlist_new();
+    list_reordered = qlist_new();
+    list_longer = qlist_new();
+    list_shorter = qlist_new();
+
+    qlist_append_int(list_0, 1);
+    qlist_append_int(list_0, 2);
+    qlist_append_int(list_0, 3);
+
+    qlist_append_int(list_1, 1);
+    qlist_append_int(list_1, 2);
+    qlist_append_int(list_1, 3);
+
+    qlist_append_int(list_reordered, 1);
+    qlist_append_int(list_reordered, 3);
+    qlist_append_int(list_reordered, 2);
+
+    qlist_append_int(list_longer, 1);
+    qlist_append_int(list_longer, 2);
+    qlist_append_int(list_longer, 3);
+    qlist_append_null(list_longer);
+
+    qlist_append_int(list_shorter, 1);
+    qlist_append_int(list_shorter, 2);
+
+    list_cloned = qlist_copy(list_0);
+
+    test_equality(true, list_0, list_1, list_cloned);
+    test_equality(false, list_0, list_reordered, list_longer, list_shorter);
+
+    /* With a NaN in it, the list should no longer compare equal to
+     * itself */
+    qlist_append(list_0, qnum_from_double(NAN));
+    g_assert(qobject_is_equal(QOBJECT(list_0), QOBJECT(list_0)) == false);
+
+    free_all(list_0, list_1, list_cloned, list_reordered, list_longer, list_shorter);
+}
+
+static void qobject_is_equal_dict_test(void)
+{
+    Error *local_err = NULL;
+    QDict *dict_0, *dict_1, *dict_cloned;
+    QDict *dict_different_key, *dict_different_value, *dict_different_null_key;
+    QDict *dict_longer, *dict_shorter, *dict_nested;
+    QDict *dict_crumpled;
+
+    dict_0 = qdict_new();
+    dict_1 = qdict_new();
+    dict_different_key = qdict_new();
+    dict_different_value = qdict_new();
+    dict_different_null_key = qdict_new();
+    dict_longer = qdict_new();
+    dict_shorter = qdict_new();
+    dict_nested = qdict_new();
+
+    qdict_put_int(dict_0, "f.o", 1);
+    qdict_put_int(dict_0, "bar", 2);
+    qdict_put_int(dict_0, "baz", 3);
+    qdict_put_null(dict_0, "null");
+
+    qdict_put_int(dict_1, "f.o", 1);
+    qdict_put_int(dict_1, "bar", 2);
+    qdict_put_int(dict_1, "baz", 3);
+    qdict_put_null(dict_1, "null");
+
+    qdict_put_int(dict_different_key, "F.o", 1);
+    qdict_put_int(dict_different_key, "bar", 2);
+    qdict_put_int(dict_different_key, "baz", 3);
+    qdict_put_null(dict_different_key, "null");
+
+    qdict_put_int(dict_different_value, "f.o", 42);
+    qdict_put_int(dict_different_value, "bar", 2);
+    qdict_put_int(dict_different_value, "baz", 3);
+    qdict_put_null(dict_different_value, "null");
+
+    qdict_put_int(dict_different_null_key, "f.o", 1);
+    qdict_put_int(dict_different_null_key, "bar", 2);
+    qdict_put_int(dict_different_null_key, "baz", 3);
+    qdict_put_null(dict_different_null_key, "none");
+
+    qdict_put_int(dict_longer, "f.o", 1);
+    qdict_put_int(dict_longer, "bar", 2);
+    qdict_put_int(dict_longer, "baz", 3);
+    qdict_put_int(dict_longer, "xyz", 4);
+    qdict_put_null(dict_longer, "null");
+
+    qdict_put_int(dict_shorter, "f.o", 1);
+    qdict_put_int(dict_shorter, "bar", 2);
+    qdict_put_int(dict_shorter, "baz", 3);
+
+    qdict_put(dict_nested, "f", qdict_new());
+    qdict_put_int(qdict_get_qdict(dict_nested, "f"), "o", 1);
+    qdict_put_int(dict_nested, "bar", 2);
+    qdict_put_int(dict_nested, "baz", 3);
+    qdict_put_null(dict_nested, "null");
+
+    dict_cloned = qdict_clone_shallow(dict_0);
+
+    test_equality(true, dict_0, dict_1, dict_cloned);
+    test_equality(false, dict_0, dict_different_key, dict_different_value,
+                         dict_different_null_key, dict_longer, dict_shorter,
+                         dict_nested);
+
+    dict_crumpled = qobject_to_qdict(qdict_crumple(dict_1, &local_err));
+    g_assert(!local_err);
+    test_equality(true, dict_crumpled, dict_nested);
+
+    qdict_flatten(dict_nested);
+    test_equality(true, dict_0, dict_nested);
+
+    /* Containing an NaN value will make this dict compare unequal to
+     * itself */
+    qdict_put(dict_0, "NaN", qnum_from_double(NAN));
+    g_assert(qobject_is_equal(QOBJECT(dict_0), QOBJECT(dict_0)) == false);
+
+    free_all(dict_0, dict_1, dict_cloned, dict_different_key,
+             dict_different_value, dict_different_null_key, dict_longer,
+             dict_shorter, dict_nested, dict_crumpled);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/public/qobject_is_equal_null",
+                    qobject_is_equal_null_test);
+    g_test_add_func("/public/qobject_is_equal_num", qobject_is_equal_num_test);
+    g_test_add_func("/public/qobject_is_equal_bool",
+                    qobject_is_equal_bool_test);
+    g_test_add_func("/public/qobject_is_equal_string",
+                    qobject_is_equal_string_test);
+    g_test_add_func("/public/qobject_is_equal_list",
+                    qobject_is_equal_list_test);
+    g_test_add_func("/public/qobject_is_equal_dict",
+                    qobject_is_equal_dict_test);
+
+    return g_test_run();
+}
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 1/6] qapi/qnull: Add own header

[Eric Blake]

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

On 09/16/2017 01:51 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h    |  1 +
>  include/qapi/qmp/qnull.h    | 30 ++++++++++++++++++++++++++++++
>  include/qapi/qmp/qobject.h  | 12 ------------
>  include/qapi/qmp/types.h    |  1 +
>  qapi/qapi-clone-visitor.c   |  1 +
>  qapi/string-input-visitor.c |  1 +
>  qobject/qnull.c             |  2 +-
>  tests/check-qnull.c         |  2 +-
>  8 files changed, 36 insertions(+), 14 deletions(-)
>  create mode 100644 include/qapi/qmp/qnull.h
> 

> +++ b/include/qapi/qmp/qnull.h
> @@ -0,0 +1,30 @@
> +/*
> + * QNull
> + *
> + * Copyright (C) 2015 Red Hat, Inc.

Worth also claiming 2017?

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v5 2/6] qapi/qlist: Add qlist_append_null() macro

[Eric Blake]

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

On 09/16/2017 01:51 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qlist.h | 3 +++
>  1 file changed, 3 insertions(+)

Please update scripts/coccinelle/qobject.cocci to also cover this (since
we updated it for qdict_put_null() in 0f9afc2).

With that addition,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v5 3/6] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 09/16/2017 01:51 PM, Max Reitz wrote:
> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +    case QNUM_DOUBLE:
> +        switch (num_y->kind) {
> +        case QNUM_I64:
> +        case QNUM_U64:
> +            return false;
> +        case QNUM_DOUBLE:
> +            /* Comparison in native double type */
> +            return num_x->u.dbl == num_y->u.dbl;

I saw comments mentioning the odd behavior of NaN; one other odd
behavior is that this treats -0.0 as equal to 0.0.  But that's fine by
me (it matches C semantics of double == double).

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v5 5/6] iotests: Add test for non-string option reopening

[Eric Blake]

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

On 09/16/2017 01:51 PM, Max Reitz wrote:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/133     | 9 +++++++++
>  tests/qemu-iotests/133.out | 5 +++++
>  2 files changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v5 6/6] tests: Add check-qobject for equality tests

[Eric Blake]

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

On 09/16/2017 01:51 PM, Max Reitz wrote:
> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
> 
> Its only purpose for now is to test the qobject_is_equal() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/Makefile.include |   4 +-
>  tests/check-qobject.c  | 315 +++++++++++++++++++++++++++++++++++++++++++++++++

We're not entirely consistent on our testsuite naming, although I'm fine
with your choice [and there's still the idea floating around in another
thread to split tests into subdirectories by whether they are part of
check-unit or check-qtest].  However, the check-* notation cannot be
automatically covered by .gitignore the way a -test suffix is, so you're
missing a change to tests/.gitignore covering the new test in an in-tree
build.

We have the idea floating around that we want to outlaw in-tree builds
altogether [although I'm not sold on that yet], and/or to generate
.gitignore, but until those land, it doesn't hurt to keep .gitignore
up-to-date.  So with that addition,

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 1/6] qapi/qnull: Add own header

[Alberto Garcia]

On Sat 16 Sep 2017 08:51:18 PM CEST, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h    |  1 +
>  include/qapi/qmp/qnull.h    | 30 ++++++++++++++++++++++++++++++
>  include/qapi/qmp/qobject.h  | 12 ------------
>  include/qapi/qmp/types.h    |  1 +
>  qapi/qapi-clone-visitor.c   |  1 +
>  qapi/string-input-visitor.c |  1 +
>  qobject/qnull.c             |  2 +-
>  tests/check-qnull.c         |  2 +-
>  8 files changed, 36 insertions(+), 14 deletions(-)
>  create mode 100644 include/qapi/qmp/qnull.h

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/6] qapi/qlist: Add qlist_append_null() macro

[Alberto Garcia]

On Sat 16 Sep 2017 08:51:19 PM CEST, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> @@ -37,6 +38,8 @@ typedef struct QList {
>          qlist_append(qlist, qbool_from_bool(value))
>  #define qlist_append_str(qlist, value) \
>          qlist_append(qlist, qstring_from_str(value))
> +#define qlist_append_null(qlist) \
> +        qlist_append(qlist, qnull())

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v5 3/6] qapi: Add qobject_is_equal()

[Alberto Garcia]

On Sat 16 Sep 2017 08:51:20 PM CEST, Max Reitz wrote:
> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.

> +bool qnull_is_equal(const QObject *x, const QObject *y);
> +

You could almost make this one inline, but it doesn't really matter.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v5 1/6] qapi/qnull: Add own header

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v5 2/6] qapi/qlist: Add qlist_append_null() macro

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v5 3/6] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v5 6/6] tests: Add check-qobject for equality tests

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
>
> Its only purpose for now is to test the qobject_is_equal() function.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/Makefile.include |   4 +-
>  tests/check-qobject.c  | 315 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 318 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qobject.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index fae5715e9c..40b8b4e98f 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -41,6 +41,7 @@ check-unit-y += tests/check-qlist$(EXESUF)
>  gcov-files-check-qlist-y = qobject/qlist.c
>  check-unit-y += tests/check-qnull$(EXESUF)
>  gcov-files-check-qnull-y = qobject/qnull.c
> +check-unit-y += tests/check-qobject$(EXESUF)
>  check-unit-y += tests/check-qjson$(EXESUF)
>  gcov-files-check-qjson-y = qobject/qjson.c
>  check-unit-y += tests/check-qlit$(EXESUF)
> @@ -542,7 +543,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \
>  	tests/test-qmp-introspect.h
>  
>  test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
> -	tests/check-qlist.o tests/check-qnull.o \
> +	tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
>  	tests/check-qjson.o tests/check-qlit.o \
>  	tests/test-coroutine.o tests/test-string-output-visitor.o \
>  	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
> @@ -576,6 +577,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
>  tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
>  tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
>  tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
> +tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
>  tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
>  tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
>  tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
> diff --git a/tests/check-qobject.c b/tests/check-qobject.c
> new file mode 100644
> index 0000000000..8f1b5550c2
> --- /dev/null
> +++ b/tests/check-qobject.c
> @@ -0,0 +1,315 @@
> +/*
> + * Generic QObject unit-tests.
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +#include "qemu/osdep.h"
> +
> +#include "qapi/qmp/types.h"
> +#include "qemu-common.h"
> +
> +#include <math.h>
> +
> +/* Marks the end of the test_equality() argument list.
> + * We cannot use NULL there because that is a valid argument. */
> +static QObject _test_equality_end_of_arguments;

Reserved identifier.  Please scratch the leading underscore.

Also: ugh!  I would've tried arrays just to avoid this ugliness.  But
since you've written it, and it works...

> +
> +/**
> + * Test whether all variadic QObject *arguments are equal (@expected
> + * is true) or whether they are all not equal (@expected is false).
> + * Every QObject is tested to be equal to itself (to test
> + * reflexivity), all tests are done both ways (to test symmetry), and
> + * transitivity is not assumed but checked (each object is compared to
> + * every other one).
> + *
> + * Note that qobject_is_equal() is not really an equivalence relation,
> + * so this function may not be used for all objects (reflexivity is
> + * not guaranteed, e.g. in the case of a QNum containing NaN).
> + */
> +static void do_test_equality(bool expected, ...)
> +{
> +    va_list ap_count, ap_extract;
> +    QObject **args;
> +    int arg_count = 0;
> +    int i, j;
> +
> +    va_start(ap_count, expected);
> +    va_copy(ap_extract, ap_count);
> +    while (va_arg(ap_count, QObject *) != &_test_equality_end_of_arguments) {
> +        arg_count++;
> +    }
> +    va_end(ap_count);
> +
> +    args = g_new(QObject *, arg_count);
> +    for (i = 0; i < arg_count; i++) {
> +        args[i] = va_arg(ap_extract, QObject *);
> +    }
> +    va_end(ap_extract);
> +
> +    for (i = 0; i < arg_count; i++) {
> +        g_assert(qobject_is_equal(args[i], args[i]) == true);
> +
> +        for (j = i + 1; j < arg_count; j++) {
> +            g_assert(qobject_is_equal(args[i], args[j]) == expected);
> +        }
> +    }
> +}
> +
> +#define test_equality(expected, ...) \
> +    do_test_equality(expected, __VA_ARGS__, &_test_equality_end_of_arguments)
> +
> +static void do_free_all(int _, ...)
> +{
> +    va_list ap;
> +    QObject *obj;
> +
> +    va_start(ap, _);
> +    while ((obj = va_arg(ap, QObject *)) != NULL) {
> +        qobject_decref(obj);
> +    }
> +    va_end(ap);
> +}
> +
> +#define free_all(...) \
> +    do_free_all(0, __VA_ARGS__, NULL)
> +
> +static void qobject_is_equal_null_test(void)
> +{
> +    test_equality(false, qnull(), NULL);
> +}
> +
> +static void qobject_is_equal_num_test(void)
> +{
> +    QNum *u0, *i0, *d0, *dnan, *um42, *im42, *dm42;
> +    QString *s0, *s_empty;
> +    QBool *bfalse;
> +
> +    u0 = qnum_from_uint(0u);
> +    i0 = qnum_from_int(0);
> +    d0 = qnum_from_double(0.0);
> +    dnan = qnum_from_double(NAN);
> +    um42 = qnum_from_uint((uint64_t)-42);
> +    im42 = qnum_from_int(-42);
> +    dm42 = qnum_from_double(-42.0);
> +
> +    s0 = qstring_from_str("0");
> +    s_empty = qstring_new();
> +    bfalse = qbool_from_bool(false);
> +
> +    /* Integers representing a mathematically equal number should
> +     * compare equal */
> +    test_equality(true, u0, i0);
> +    /* Doubles, however, are always unequal to integers */
> +    test_equality(false, u0, d0);
> +    test_equality(false, i0, d0);

Would the following be more legible?

       /* Integers representing a mathematically equal number should
        * compare equal */
       check_equal(u0, i0);
       /* Doubles, however, are always unequal to integers */
       check_unequal(u0, d0);
       check_unequal(i0, d0);

(with the obvious macros check_equal() and check_unequal())

> +    /* No automatic type conversion */
> +    test_equality(false, u0, s0, s_empty, bfalse, qnull(), NULL);
> +    test_equality(false, i0, s0, s_empty, bfalse, qnull(), NULL);
> +    test_equality(false, d0, s0, s_empty, bfalse, qnull(), NULL);

No "num" in this part.  No big deal, but perhaps you'd like to put it
into a qobject_is_equal_mixed_test().

> +
> +    /* Do not assume any object is equal to itself -- note however
> +     * that NaN cannot occur in a JSON object anyway. */
> +    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
> +
> +    /* No unsigned overflow */
> +    test_equality(false, um42, im42);
> +    test_equality(false, um42, dm42);
> +    g_assert(qobject_is_equal(QOBJECT(im42), QOBJECT(dm42)) == false);
> +    test_equality(false, im42, dm42);
> +
> +

Scratch one blank line.

> +    free_all(u0, i0, d0, dnan, um42, im42, dm42, s0, s_empty, bfalse);
> +}
> +
> +static void qobject_is_equal_bool_test(void)
> +{
> +    QBool *btrue_0, *btrue_1, *bfalse_0, *bfalse_1;
> +
> +    /* Automatic type conversion is tested in the QNum test */
> +
> +    btrue_0 = qbool_from_bool(true);
> +    btrue_1 = qbool_from_bool(true);
> +    bfalse_0 = qbool_from_bool(false);
> +    bfalse_1 = qbool_from_bool(false);
> +
> +    test_equality(true, btrue_0, btrue_1);
> +    test_equality(true, bfalse_0, bfalse_1);
> +    test_equality(false, btrue_0, bfalse_0);
> +    test_equality(false, btrue_1, bfalse_1);

The last one is kind of redundant.  Your choice.

> +
> +    free_all(btrue_0, btrue_1, bfalse_0, bfalse_1);
> +}
> +
> +static void qobject_is_equal_string_test(void)
> +{
> +    QString *str_base, *str_whitespace_0, *str_whitespace_1, *str_whitespace_2;
> +    QString *str_whitespace_3, *str_case, *str_built;
> +
> +    str_base = qstring_from_str("foo");
> +    str_whitespace_0 = qstring_from_str(" foo");
> +    str_whitespace_1 = qstring_from_str("foo ");
> +    str_whitespace_2 = qstring_from_str("foo\b");
> +    str_whitespace_3 = qstring_from_str("fooo\b");
> +    str_case = qstring_from_str("Foo");
> +
> +    /* Should yield "foo" */
> +    str_built = qstring_from_substr("form", 0, 1);
> +    qstring_append_chr(str_built, 'o');
> +
> +    test_equality(false, str_base, str_whitespace_0, str_whitespace_1,
> +                         str_whitespace_2, str_whitespace_3, str_case);
> +
> +    test_equality(true, str_base, str_built);
> +
> +    free_all(str_base, str_whitespace_0, str_whitespace_1, str_whitespace_2,
> +             str_whitespace_3, str_case, str_built);
> +}
> +
> +static void qobject_is_equal_list_test(void)
> +{
> +    QList *list_0, *list_1, *list_cloned;
> +    QList *list_reordered, *list_longer, *list_shorter;
> +
> +    list_0 = qlist_new();
> +    list_1 = qlist_new();
> +    list_reordered = qlist_new();
> +    list_longer = qlist_new();
> +    list_shorter = qlist_new();
> +
> +    qlist_append_int(list_0, 1);
> +    qlist_append_int(list_0, 2);
> +    qlist_append_int(list_0, 3);
> +
> +    qlist_append_int(list_1, 1);
> +    qlist_append_int(list_1, 2);
> +    qlist_append_int(list_1, 3);
> +
> +    qlist_append_int(list_reordered, 1);
> +    qlist_append_int(list_reordered, 3);
> +    qlist_append_int(list_reordered, 2);
> +
> +    qlist_append_int(list_longer, 1);
> +    qlist_append_int(list_longer, 2);
> +    qlist_append_int(list_longer, 3);
> +    qlist_append_null(list_longer);
> +
> +    qlist_append_int(list_shorter, 1);
> +    qlist_append_int(list_shorter, 2);
> +
> +    list_cloned = qlist_copy(list_0);
> +
> +    test_equality(true, list_0, list_1, list_cloned);
> +    test_equality(false, list_0, list_reordered, list_longer, list_shorter);
> +
> +    /* With a NaN in it, the list should no longer compare equal to
> +     * itself */
> +    qlist_append(list_0, qnum_from_double(NAN));
> +    g_assert(qobject_is_equal(QOBJECT(list_0), QOBJECT(list_0)) == false);
> +
> +    free_all(list_0, list_1, list_cloned, list_reordered, list_longer, list_shorter);
> +}
> +
> +static void qobject_is_equal_dict_test(void)
> +{
> +    Error *local_err = NULL;
> +    QDict *dict_0, *dict_1, *dict_cloned;
> +    QDict *dict_different_key, *dict_different_value, *dict_different_null_key;
> +    QDict *dict_longer, *dict_shorter, *dict_nested;
> +    QDict *dict_crumpled;
> +
> +    dict_0 = qdict_new();
> +    dict_1 = qdict_new();
> +    dict_different_key = qdict_new();
> +    dict_different_value = qdict_new();
> +    dict_different_null_key = qdict_new();
> +    dict_longer = qdict_new();
> +    dict_shorter = qdict_new();
> +    dict_nested = qdict_new();
> +
> +    qdict_put_int(dict_0, "f.o", 1);
> +    qdict_put_int(dict_0, "bar", 2);
> +    qdict_put_int(dict_0, "baz", 3);
> +    qdict_put_null(dict_0, "null");
> +
> +    qdict_put_int(dict_1, "f.o", 1);
> +    qdict_put_int(dict_1, "bar", 2);
> +    qdict_put_int(dict_1, "baz", 3);
> +    qdict_put_null(dict_1, "null");
> +
> +    qdict_put_int(dict_different_key, "F.o", 1);
> +    qdict_put_int(dict_different_key, "bar", 2);
> +    qdict_put_int(dict_different_key, "baz", 3);
> +    qdict_put_null(dict_different_key, "null");
> +
> +    qdict_put_int(dict_different_value, "f.o", 42);
> +    qdict_put_int(dict_different_value, "bar", 2);
> +    qdict_put_int(dict_different_value, "baz", 3);
> +    qdict_put_null(dict_different_value, "null");
> +
> +    qdict_put_int(dict_different_null_key, "f.o", 1);
> +    qdict_put_int(dict_different_null_key, "bar", 2);
> +    qdict_put_int(dict_different_null_key, "baz", 3);
> +    qdict_put_null(dict_different_null_key, "none");
> +
> +    qdict_put_int(dict_longer, "f.o", 1);
> +    qdict_put_int(dict_longer, "bar", 2);
> +    qdict_put_int(dict_longer, "baz", 3);
> +    qdict_put_int(dict_longer, "xyz", 4);
> +    qdict_put_null(dict_longer, "null");
> +
> +    qdict_put_int(dict_shorter, "f.o", 1);
> +    qdict_put_int(dict_shorter, "bar", 2);
> +    qdict_put_int(dict_shorter, "baz", 3);
> +
> +    qdict_put(dict_nested, "f", qdict_new());
> +    qdict_put_int(qdict_get_qdict(dict_nested, "f"), "o", 1);
> +    qdict_put_int(dict_nested, "bar", 2);
> +    qdict_put_int(dict_nested, "baz", 3);
> +    qdict_put_null(dict_nested, "null");
> +
> +    dict_cloned = qdict_clone_shallow(dict_0);
> +
> +    test_equality(true, dict_0, dict_1, dict_cloned);
> +    test_equality(false, dict_0, dict_different_key, dict_different_value,
> +                         dict_different_null_key, dict_longer, dict_shorter,
> +                         dict_nested);
> +
> +    dict_crumpled = qobject_to_qdict(qdict_crumple(dict_1, &local_err));
> +    g_assert(!local_err);
> +    test_equality(true, dict_crumpled, dict_nested);
> +
> +    qdict_flatten(dict_nested);
> +    test_equality(true, dict_0, dict_nested);
> +
> +    /* Containing an NaN value will make this dict compare unequal to
> +     * itself */
> +    qdict_put(dict_0, "NaN", qnum_from_double(NAN));
> +    g_assert(qobject_is_equal(QOBJECT(dict_0), QOBJECT(dict_0)) == false);
> +
> +    free_all(dict_0, dict_1, dict_cloned, dict_different_key,
> +             dict_different_value, dict_different_null_key, dict_longer,
> +             dict_shorter, dict_nested, dict_crumpled);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    g_test_add_func("/public/qobject_is_equal_null",
> +                    qobject_is_equal_null_test);
> +    g_test_add_func("/public/qobject_is_equal_num", qobject_is_equal_num_test);
> +    g_test_add_func("/public/qobject_is_equal_bool",
> +                    qobject_is_equal_bool_test);
> +    g_test_add_func("/public/qobject_is_equal_string",
> +                    qobject_is_equal_string_test);
> +    g_test_add_func("/public/qobject_is_equal_list",
> +                    qobject_is_equal_list_test);
> +    g_test_add_func("/public/qobject_is_equal_dict",
> +                    qobject_is_equal_dict_test);
> +
> +    return g_test_run();
> +}

Quite thorough :)

With at least the reserved identifier renamed and the extra blank line
scratched:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v5 6/6] tests: Add check-qobject for equality tests

[Max Reitz]

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

On 2017-10-02 15:34, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Add a new test file (check-qobject.c) for unit tests that concern
>> QObjects as a whole.
>>
>> Its only purpose for now is to test the qobject_is_equal() function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/Makefile.include |   4 +-
>>  tests/check-qobject.c  | 315 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 318 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/check-qobject.c
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index fae5715e9c..40b8b4e98f 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -41,6 +41,7 @@ check-unit-y += tests/check-qlist$(EXESUF)
>>  gcov-files-check-qlist-y = qobject/qlist.c
>>  check-unit-y += tests/check-qnull$(EXESUF)
>>  gcov-files-check-qnull-y = qobject/qnull.c
>> +check-unit-y += tests/check-qobject$(EXESUF)
>>  check-unit-y += tests/check-qjson$(EXESUF)
>>  gcov-files-check-qjson-y = qobject/qjson.c
>>  check-unit-y += tests/check-qlit$(EXESUF)
>> @@ -542,7 +543,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \
>>  	tests/test-qmp-introspect.h
>>  
>>  test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
>> -	tests/check-qlist.o tests/check-qnull.o \
>> +	tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
>>  	tests/check-qjson.o tests/check-qlit.o \
>>  	tests/test-coroutine.o tests/test-string-output-visitor.o \
>>  	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
>> @@ -576,6 +577,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
>>  tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
>>  tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
>>  tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
>> +tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
>>  tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
>>  tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
>>  tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
>> diff --git a/tests/check-qobject.c b/tests/check-qobject.c
>> new file mode 100644
>> index 0000000000..8f1b5550c2
>> --- /dev/null
>> +++ b/tests/check-qobject.c
>> @@ -0,0 +1,315 @@
>> +/*
>> + * Generic QObject unit-tests.
>> + *
>> + * Copyright (C) 2017 Red Hat Inc.
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + */
>> +#include "qemu/osdep.h"
>> +
>> +#include "qapi/qmp/types.h"
>> +#include "qemu-common.h"
>> +
>> +#include <math.h>
>> +
>> +/* Marks the end of the test_equality() argument list.
>> + * We cannot use NULL there because that is a valid argument. */
>> +static QObject _test_equality_end_of_arguments;
> 
> Reserved identifier.  Please scratch the leading underscore.

OK.

> Also: ugh!  I would've tried arrays just to avoid this ugliness.  But
> since you've written it, and it works...

*cough*

>> +
>> +/**
>> + * Test whether all variadic QObject *arguments are equal (@expected
>> + * is true) or whether they are all not equal (@expected is false).
>> + * Every QObject is tested to be equal to itself (to test
>> + * reflexivity), all tests are done both ways (to test symmetry), and
>> + * transitivity is not assumed but checked (each object is compared to
>> + * every other one).
>> + *
>> + * Note that qobject_is_equal() is not really an equivalence relation,
>> + * so this function may not be used for all objects (reflexivity is
>> + * not guaranteed, e.g. in the case of a QNum containing NaN).
>> + */
>> +static void do_test_equality(bool expected, ...)
>> +{
>> +    va_list ap_count, ap_extract;
>> +    QObject **args;
>> +    int arg_count = 0;
>> +    int i, j;
>> +
>> +    va_start(ap_count, expected);
>> +    va_copy(ap_extract, ap_count);
>> +    while (va_arg(ap_count, QObject *) != &_test_equality_end_of_arguments) {
>> +        arg_count++;
>> +    }
>> +    va_end(ap_count);
>> +
>> +    args = g_new(QObject *, arg_count);
>> +    for (i = 0; i < arg_count; i++) {
>> +        args[i] = va_arg(ap_extract, QObject *);
>> +    }
>> +    va_end(ap_extract);
>> +
>> +    for (i = 0; i < arg_count; i++) {
>> +        g_assert(qobject_is_equal(args[i], args[i]) == true);
>> +
>> +        for (j = i + 1; j < arg_count; j++) {
>> +            g_assert(qobject_is_equal(args[i], args[j]) == expected);
>> +        }
>> +    }
>> +}
>> +
>> +#define test_equality(expected, ...) \
>> +    do_test_equality(expected, __VA_ARGS__, &_test_equality_end_of_arguments)
>> +
>> +static void do_free_all(int _, ...)
>> +{
>> +    va_list ap;
>> +    QObject *obj;
>> +
>> +    va_start(ap, _);
>> +    while ((obj = va_arg(ap, QObject *)) != NULL) {
>> +        qobject_decref(obj);
>> +    }
>> +    va_end(ap);
>> +}
>> +
>> +#define free_all(...) \
>> +    do_free_all(0, __VA_ARGS__, NULL)
>> +
>> +static void qobject_is_equal_null_test(void)
>> +{
>> +    test_equality(false, qnull(), NULL);
>> +}
>> +
>> +static void qobject_is_equal_num_test(void)
>> +{
>> +    QNum *u0, *i0, *d0, *dnan, *um42, *im42, *dm42;
>> +    QString *s0, *s_empty;
>> +    QBool *bfalse;
>> +
>> +    u0 = qnum_from_uint(0u);
>> +    i0 = qnum_from_int(0);
>> +    d0 = qnum_from_double(0.0);
>> +    dnan = qnum_from_double(NAN);
>> +    um42 = qnum_from_uint((uint64_t)-42);
>> +    im42 = qnum_from_int(-42);
>> +    dm42 = qnum_from_double(-42.0);
>> +
>> +    s0 = qstring_from_str("0");
>> +    s_empty = qstring_new();
>> +    bfalse = qbool_from_bool(false);
>> +
>> +    /* Integers representing a mathematically equal number should
>> +     * compare equal */
>> +    test_equality(true, u0, i0);
>> +    /* Doubles, however, are always unequal to integers */
>> +    test_equality(false, u0, d0);
>> +    test_equality(false, i0, d0);
> 
> Would the following be more legible?
> 
>        /* Integers representing a mathematically equal number should
>         * compare equal */
>        check_equal(u0, i0);
>        /* Doubles, however, are always unequal to integers */
>        check_unequal(u0, d0);
>        check_unequal(i0, d0);
> 
> (with the obvious macros check_equal() and check_unequal())

Sounds good, yes.

>> +    /* No automatic type conversion */
>> +    test_equality(false, u0, s0, s_empty, bfalse, qnull(), NULL);
>> +    test_equality(false, i0, s0, s_empty, bfalse, qnull(), NULL);
>> +    test_equality(false, d0, s0, s_empty, bfalse, qnull(), NULL);
> 
> No "num" in this part.  No big deal, but perhaps you'd like to put it
> into a qobject_is_equal_mixed_test().

Hm? u0, i0, and d0 are QNums.  Adding a mixed test for type conversion
still sounds like a good idea, though.

>> +
>> +    /* Do not assume any object is equal to itself -- note however
>> +     * that NaN cannot occur in a JSON object anyway. */
>> +    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
>> +
>> +    /* No unsigned overflow */
>> +    test_equality(false, um42, im42);
>> +    test_equality(false, um42, dm42);
>> +    g_assert(qobject_is_equal(QOBJECT(im42), QOBJECT(dm42)) == false);
>> +    test_equality(false, im42, dm42);
>> +
>> +
> 
> Scratch one blank line.

Will do.

>> +    free_all(u0, i0, d0, dnan, um42, im42, dm42, s0, s_empty, bfalse);
>> +}
>> +
>> +static void qobject_is_equal_bool_test(void)
>> +{
>> +    QBool *btrue_0, *btrue_1, *bfalse_0, *bfalse_1;
>> +
>> +    /* Automatic type conversion is tested in the QNum test */
>> +
>> +    btrue_0 = qbool_from_bool(true);
>> +    btrue_1 = qbool_from_bool(true);
>> +    bfalse_0 = qbool_from_bool(false);
>> +    bfalse_1 = qbool_from_bool(false);
>> +
>> +    test_equality(true, btrue_0, btrue_1);
>> +    test_equality(true, bfalse_0, bfalse_1);
>> +    test_equality(false, btrue_0, bfalse_0);
>> +    test_equality(false, btrue_1, bfalse_1);
> 
> The last one is kind of redundant.  Your choice.

Hmmm, yes. :-)

>> +
>> +    free_all(btrue_0, btrue_1, bfalse_0, bfalse_1);
>> +}
>> +
>> +static void qobject_is_equal_string_test(void)
>> +{
>> +    QString *str_base, *str_whitespace_0, *str_whitespace_1, *str_whitespace_2;
>> +    QString *str_whitespace_3, *str_case, *str_built;
>> +
>> +    str_base = qstring_from_str("foo");
>> +    str_whitespace_0 = qstring_from_str(" foo");
>> +    str_whitespace_1 = qstring_from_str("foo ");
>> +    str_whitespace_2 = qstring_from_str("foo\b");
>> +    str_whitespace_3 = qstring_from_str("fooo\b");
>> +    str_case = qstring_from_str("Foo");
>> +
>> +    /* Should yield "foo" */
>> +    str_built = qstring_from_substr("form", 0, 1);
>> +    qstring_append_chr(str_built, 'o');
>> +
>> +    test_equality(false, str_base, str_whitespace_0, str_whitespace_1,
>> +                         str_whitespace_2, str_whitespace_3, str_case);
>> +
>> +    test_equality(true, str_base, str_built);
>> +
>> +    free_all(str_base, str_whitespace_0, str_whitespace_1, str_whitespace_2,
>> +             str_whitespace_3, str_case, str_built);
>> +}
>> +
>> +static void qobject_is_equal_list_test(void)
>> +{
>> +    QList *list_0, *list_1, *list_cloned;
>> +    QList *list_reordered, *list_longer, *list_shorter;
>> +
>> +    list_0 = qlist_new();
>> +    list_1 = qlist_new();
>> +    list_reordered = qlist_new();
>> +    list_longer = qlist_new();
>> +    list_shorter = qlist_new();
>> +
>> +    qlist_append_int(list_0, 1);
>> +    qlist_append_int(list_0, 2);
>> +    qlist_append_int(list_0, 3);
>> +
>> +    qlist_append_int(list_1, 1);
>> +    qlist_append_int(list_1, 2);
>> +    qlist_append_int(list_1, 3);
>> +
>> +    qlist_append_int(list_reordered, 1);
>> +    qlist_append_int(list_reordered, 3);
>> +    qlist_append_int(list_reordered, 2);
>> +
>> +    qlist_append_int(list_longer, 1);
>> +    qlist_append_int(list_longer, 2);
>> +    qlist_append_int(list_longer, 3);
>> +    qlist_append_null(list_longer);
>> +
>> +    qlist_append_int(list_shorter, 1);
>> +    qlist_append_int(list_shorter, 2);
>> +
>> +    list_cloned = qlist_copy(list_0);
>> +
>> +    test_equality(true, list_0, list_1, list_cloned);
>> +    test_equality(false, list_0, list_reordered, list_longer, list_shorter);
>> +
>> +    /* With a NaN in it, the list should no longer compare equal to
>> +     * itself */
>> +    qlist_append(list_0, qnum_from_double(NAN));
>> +    g_assert(qobject_is_equal(QOBJECT(list_0), QOBJECT(list_0)) == false);
>> +
>> +    free_all(list_0, list_1, list_cloned, list_reordered, list_longer, list_shorter);
>> +}
>> +
>> +static void qobject_is_equal_dict_test(void)
>> +{
>> +    Error *local_err = NULL;
>> +    QDict *dict_0, *dict_1, *dict_cloned;
>> +    QDict *dict_different_key, *dict_different_value, *dict_different_null_key;
>> +    QDict *dict_longer, *dict_shorter, *dict_nested;
>> +    QDict *dict_crumpled;
>> +
>> +    dict_0 = qdict_new();
>> +    dict_1 = qdict_new();
>> +    dict_different_key = qdict_new();
>> +    dict_different_value = qdict_new();
>> +    dict_different_null_key = qdict_new();
>> +    dict_longer = qdict_new();
>> +    dict_shorter = qdict_new();
>> +    dict_nested = qdict_new();
>> +
>> +    qdict_put_int(dict_0, "f.o", 1);
>> +    qdict_put_int(dict_0, "bar", 2);
>> +    qdict_put_int(dict_0, "baz", 3);
>> +    qdict_put_null(dict_0, "null");
>> +
>> +    qdict_put_int(dict_1, "f.o", 1);
>> +    qdict_put_int(dict_1, "bar", 2);
>> +    qdict_put_int(dict_1, "baz", 3);
>> +    qdict_put_null(dict_1, "null");
>> +
>> +    qdict_put_int(dict_different_key, "F.o", 1);
>> +    qdict_put_int(dict_different_key, "bar", 2);
>> +    qdict_put_int(dict_different_key, "baz", 3);
>> +    qdict_put_null(dict_different_key, "null");
>> +
>> +    qdict_put_int(dict_different_value, "f.o", 42);
>> +    qdict_put_int(dict_different_value, "bar", 2);
>> +    qdict_put_int(dict_different_value, "baz", 3);
>> +    qdict_put_null(dict_different_value, "null");
>> +
>> +    qdict_put_int(dict_different_null_key, "f.o", 1);
>> +    qdict_put_int(dict_different_null_key, "bar", 2);
>> +    qdict_put_int(dict_different_null_key, "baz", 3);
>> +    qdict_put_null(dict_different_null_key, "none");
>> +
>> +    qdict_put_int(dict_longer, "f.o", 1);
>> +    qdict_put_int(dict_longer, "bar", 2);
>> +    qdict_put_int(dict_longer, "baz", 3);
>> +    qdict_put_int(dict_longer, "xyz", 4);
>> +    qdict_put_null(dict_longer, "null");
>> +
>> +    qdict_put_int(dict_shorter, "f.o", 1);
>> +    qdict_put_int(dict_shorter, "bar", 2);
>> +    qdict_put_int(dict_shorter, "baz", 3);
>> +
>> +    qdict_put(dict_nested, "f", qdict_new());
>> +    qdict_put_int(qdict_get_qdict(dict_nested, "f"), "o", 1);
>> +    qdict_put_int(dict_nested, "bar", 2);
>> +    qdict_put_int(dict_nested, "baz", 3);
>> +    qdict_put_null(dict_nested, "null");
>> +
>> +    dict_cloned = qdict_clone_shallow(dict_0);
>> +
>> +    test_equality(true, dict_0, dict_1, dict_cloned);
>> +    test_equality(false, dict_0, dict_different_key, dict_different_value,
>> +                         dict_different_null_key, dict_longer, dict_shorter,
>> +                         dict_nested);
>> +
>> +    dict_crumpled = qobject_to_qdict(qdict_crumple(dict_1, &local_err));
>> +    g_assert(!local_err);
>> +    test_equality(true, dict_crumpled, dict_nested);
>> +
>> +    qdict_flatten(dict_nested);
>> +    test_equality(true, dict_0, dict_nested);
>> +
>> +    /* Containing an NaN value will make this dict compare unequal to
>> +     * itself */
>> +    qdict_put(dict_0, "NaN", qnum_from_double(NAN));
>> +    g_assert(qobject_is_equal(QOBJECT(dict_0), QOBJECT(dict_0)) == false);
>> +
>> +    free_all(dict_0, dict_1, dict_cloned, dict_different_key,
>> +             dict_different_value, dict_different_null_key, dict_longer,
>> +             dict_shorter, dict_nested, dict_crumpled);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    g_test_add_func("/public/qobject_is_equal_null",
>> +                    qobject_is_equal_null_test);
>> +    g_test_add_func("/public/qobject_is_equal_num", qobject_is_equal_num_test);
>> +    g_test_add_func("/public/qobject_is_equal_bool",
>> +                    qobject_is_equal_bool_test);
>> +    g_test_add_func("/public/qobject_is_equal_string",
>> +                    qobject_is_equal_string_test);
>> +    g_test_add_func("/public/qobject_is_equal_list",
>> +                    qobject_is_equal_list_test);
>> +    g_test_add_func("/public/qobject_is_equal_dict",
>> +                    qobject_is_equal_dict_test);
>> +
>> +    return g_test_run();
>> +}
> 
> Quite thorough :)
> 
> With at least the reserved identifier renamed and the extra blank line
> scratched:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for your reviews!

Max


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