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

[Qemu-devel] [PATCH v3 0/5] 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.


v3:
- Patch 1:
  - Fix copyright header [Markus]
  - Drop qobject.h include from qnull.c [Markus]
  - Replacing all QObject includes by qmp/types.h in target/i386/cpu.c
    became unnecessary due to 01b2ffcedd94ad7b42bc870e4c6936c87ad03429
    (kept R-b anyway, because v2 contained exactly the same change as
     that commit)
- Patch 2:
  - Move the code for QInt and QFloat to QNum
  - Write function descriptions in imperative mood [Markus]
  - Break comment lines after 70 characters [Markus]
  - Style changes [Markus]
- Patch 3: Add a comment on what will go wrong when using e.g. -blockdev
           and the qemu-io reopen command together [Markus]
- Patch 5: Added some test cases for qobject_is_equal() [Markus]


git-backport-diff against v2:

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/5:[0017] [FC] 'qapi/qnull: Add own header'
002/5:[0120] [FC] 'qapi: Add qobject_is_equal()'
003/5:[0018] [FC] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
004/5:[----] [--] 'iotests: Add test for non-string option reopening'
005/5:[down] 'tests: Add check-qobject for equality tests'


Max Reitz (5):
  qapi/qnull: Add own header
  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   |   1 +
 include/qapi/qmp/qlist.h   |   1 +
 include/qapi/qmp/qnull.h   |  28 ++++
 include/qapi/qmp/qnum.h    |   1 +
 include/qapi/qmp/qobject.h |  17 +--
 include/qapi/qmp/qstring.h |   1 +
 include/qapi/qmp/types.h   |   1 +
 block.c                    |  31 +++--
 qobject/qbool.c            |   8 ++
 qobject/qdict.c            |  29 +++++
 qobject/qlist.c            |  32 +++++
 qobject/qnull.c            |  11 +-
 qobject/qnum.c             |  53 ++++++++
 qobject/qobject.c          |  29 +++++
 qobject/qstring.c          |   9 ++
 tests/check-qnull.c        |   2 +-
 tests/check-qobject.c      | 312 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/133     |   9 ++
 tests/qemu-iotests/133.out |   5 +
 21 files changed, 561 insertions(+), 24 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.9.4

[Qemu-devel] [PATCH v3 1/5] qapi/qnull: Add own header

[Max Reitz]

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qnull.h   | 26 ++++++++++++++++++++++++++
 include/qapi/qmp/qobject.h |  8 --------
 include/qapi/qmp/types.h   |  1 +
 qobject/qnull.c            |  2 +-
 tests/check-qnull.c        |  2 +-
 5 files changed, 29 insertions(+), 10 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h

diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
new file mode 100644
index 0000000..48edad4
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,26 @@
+/*
+ * 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"
+
+extern QObject qnull_;
+
+static inline QObject *qnull(void)
+{
+    qobject_incref(&qnull_);
+    return &qnull_;
+}
+
+#endif /* QNULL_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index b8ddbca..ef1d1a9 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,12 +93,4 @@ static inline QType qobject_type(const QObject *obj)
     return obj->type;
 }
 
-extern QObject qnull_;
-
-static inline QObject *qnull(void)
-{
-    qobject_incref(&qnull_);
-    return &qnull_;
-}
-
 #endif /* QOBJECT_H */
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index a4bc662..749ac44 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/qobject/qnull.c b/qobject/qnull.c
index c124d05..43918f1 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"
 
 QObject qnull_ = {
     .type = QTYPE_QNULL,
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 8dd1c96..4a67b9a 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.9.4

Re: [Qemu-devel] [PATCH v3 1/5] qapi/qnull: Add own header

[Eric Blake]

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

On 07/03/2017 07:25 AM, Max Reitz wrote:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qnull.h   | 26 ++++++++++++++++++++++++++
>  include/qapi/qmp/qobject.h |  8 --------
>  include/qapi/qmp/types.h   |  1 +
>  qobject/qnull.c            |  2 +-
>  tests/check-qnull.c        |  2 +-
>  5 files changed, 29 insertions(+), 10 deletions(-)
>  create mode 100644 include/qapi/qmp/qnull.h
> 
> diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
> new file mode 100644
> index 0000000..48edad4
> --- /dev/null
> +++ b/include/qapi/qmp/qnull.h
> @@ -0,0 +1,26 @@
> +/*
> + * QNull
> + *
> + * Copyright (C) 2015 Red Hat, Inc.

The date makes sense based on when the bulk of the content for this file
was first introduced elsewhere, but I'm okay if you also want to add 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: 604 bytes --]

[Qemu-devel] [PATCH v3 2/5] 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             | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 qobject/qobject.c          | 29 +++++++++++++++++++++++++
 qobject/qstring.c          |  9 ++++++++
 14 files changed, 185 insertions(+)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index a41111c..f77ea86 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 363e431..84f8ea7 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -42,6 +42,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 c4b5fda..24e1e9f 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -58,6 +58,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 48edad4..f4fbcae 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -23,4 +23,6 @@ static inline QObject *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 09d745c..237d01b 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -48,6 +48,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 ef1d1a9..38ac688 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 10076b7..65c05a9 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 0606bbd..ac825fc 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 576018e..e8f15f1 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 86b60cb..3ef57d3 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 43918f1..4b9cdbc 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -18,3 +18,12 @@ QObject qnull_ = {
     .type = QTYPE_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 476e81c..784d061 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
 }
 
 /**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ */
+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:
+            /* Implicit conversion of x to double; no overflow
+             * possible */
+            return num_x->u.i64 == num_y->u.dbl;
+        }
+        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:
+            /* Implicit conversion of x to double; no overflow
+             * possible */
+            return num_x->u.u64 == num_y->u.dbl;
+        }
+        abort();
+    case QNUM_DOUBLE:
+        switch (num_y->kind) {
+        case QNUM_I64:
+            return qnum_is_equal(y, x);
+        case QNUM_U64:
+            return qnum_is_equal(y, x);
+        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 b0cafb6..b2a5360 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 5da7b5f..74182a1 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.9.4

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/03/2017 07:25 AM, 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>
> ---

> +++ b/qobject/qnum.c

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

Which returns false if we allow NaN (in general, JSON does not, so I'm
not sure if we allow NaN in a QNum)

> +++ b/qobject/qobject.c

> +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). */

But this comment matches the code above in QNum.

> +
> +    if (!x && !y) {
> +        return true;
> +    }
> +
> +    if (!x || !y || x->type != y->type) {
> +        return false;
> +    }

I like that there is no implicit conversion between bool and int; the
only conversions that still allow equality are within the subtypes of QNum.

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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] 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>
[...]
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 476e81c..784d061 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>  }
>  
>  /**
> + * qnum_is_equal(): Test whether the two QNums are equal
> + */
> +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:
> +            /* Implicit conversion of x to double; no overflow
> +             * possible */
> +            return num_x->u.i64 == num_y->u.dbl;

Overflow is impossible, but loss of precision is possible:

    (double)9007199254740993ull == 9007199254740992.0

yields true.  Is this what we want?

> +        }
> +        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:
> +            /* Implicit conversion of x to double; no overflow
> +             * possible */
> +            return num_x->u.u64 == num_y->u.dbl;

Similar loss of precision.

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

I think there's more than one sane interpretations of "is equal",
including:

* The mathematical numbers represented by @x and @y are equal.

* @x and @y have the same contents, i.e. same kind and u.

* @x and @y are the same object (listed for completeness; we don't need
  a function to compare pointers).

Your patch implements yet another one.  Which one do we want, and why?

The second is easier to implement than the first.

If we really want the first, you need to fix the loss of precision bugs.
I guess the obvious fix is

    return (double)x == x && x == y;

Note that this is what you do for mixed signedness: first check @x is
exactly representable in @y's type, then compare in @y's type.

Regardless of which one we pick, the function comment needs to explain.

> +
> +/**
>   * qnum_destroy_obj(): Free all memory allocated by a
>   * QNum object
>   */
[...]

Remainder of the patch looks good to me.

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 09:07, Markus Armbruster wrote:
> 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>
> [...]
>> diff --git a/qobject/qnum.c b/qobject/qnum.c
>> index 476e81c..784d061 100644
>> --- a/qobject/qnum.c
>> +++ b/qobject/qnum.c
>> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qnum_is_equal(): Test whether the two QNums are equal
>> + */
>> +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:
>> +            /* Implicit conversion of x to double; no overflow
>> +             * possible */
>> +            return num_x->u.i64 == num_y->u.dbl;
> 
> Overflow is impossible, but loss of precision is possible:
> 
>     (double)9007199254740993ull == 9007199254740992.0
> 
> yields true.  Is this what we want?

I'd argue that yes, because the floating point value represents
basically all of the values which are "equal" to it.

But I don't have a string opinion. I guess the alternative would be to
convert the double to an integer instead and check for overflows before?

>> +        }
>> +        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:
>> +            /* Implicit conversion of x to double; no overflow
>> +             * possible */
>> +            return num_x->u.u64 == num_y->u.dbl;
> 
> Similar loss of precision.
> 
>> +        }
>> +        abort();
>> +    case QNUM_DOUBLE:
>> +        switch (num_y->kind) {
>> +        case QNUM_I64:
>> +            return qnum_is_equal(y, x);
>> +        case QNUM_U64:
>> +            return qnum_is_equal(y, x);
>> +        case QNUM_DOUBLE:
>> +            /* Comparison in native double type */
>> +            return num_x->u.dbl == num_y->u.dbl;
>> +        }
>> +        abort();
>> +    }
>> +
>> +    abort();
>> +}
> 
> I think there's more than one sane interpretations of "is equal",
> including:
> 
> * The mathematical numbers represented by @x and @y are equal.
> 
> * @x and @y have the same contents, i.e. same kind and u.
> 
> * @x and @y are the same object (listed for completeness; we don't need
>   a function to compare pointers).
> 
> Your patch implements yet another one.  Which one do we want, and why?

Mine is the first one, except that I think that a floating point value
does not represent a single number but just some number in a range.

> The second is easier to implement than the first.

It seems much less useful, though.

> If we really want the first, you need to fix the loss of precision bugs.

I'm not sure, but I don't mind either, so...

> I guess the obvious fix is
> 
>     return (double)x == x && x == y;

Yes, that would do, too; and spares me of having to think about how well
comparing an arbitrary double to UINT64_MAX actually works. :-)

> Note that this is what you do for mixed signedness: first check @x is
> exactly representable in @y's type, then compare in @y's type.
> 
> Regardless of which one we pick, the function comment needs to explain.

OK, will do.

Max

>> +
>> +/**
>>   * qnum_destroy_obj(): Free all memory allocated by a
>>   * QNum object
>>   */
> [...]
> 
> Remainder of the patch looks good to me.
> 



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

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/05/2017 08:48 AM, Max Reitz wrote:
>>>  /**
>>> + * qnum_is_equal(): Test whether the two QNums are equal
>>> + */
>>> +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:
>>> +            /* Implicit conversion of x to double; no overflow
>>> +             * possible */
>>> +            return num_x->u.i64 == num_y->u.dbl;
>>
>> Overflow is impossible, but loss of precision is possible:
>>
>>     (double)9007199254740993ull == 9007199254740992.0
>>
>> yields true.  Is this what we want?
> 
> I'd argue that yes, because the floating point value represents
> basically all of the values which are "equal" to it.

But the problem is that we CAN represent the fully-precise number as an
integer, so having multiple distinct integers that compare differently
against each other, but equal to the same double, is awkward.

> 
> But I don't have a string opinion. I guess the alternative would be to
> convert the double to an integer instead and check for overflows before?

That's the solution Markus gave, and I'm in favor of the tighter check:

> 
>> I guess the obvious fix is
>>
>>     return (double)x == x && x == y;
> 
> Yes, that would do, too; and spares me of having to think about how well
> comparing an arbitrary double to UINT64_MAX actually works. :-)

It basically says that we are unwilling to declare an integer equivalent
to the double if the double loses precision when trying to store the
integer.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/05/2017 08:48 AM, Max Reitz wrote:
>>>>  /**
>>>> + * qnum_is_equal(): Test whether the two QNums are equal
>>>> + */
>>>> +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:
>>>> +            /* Implicit conversion of x to double; no overflow
>>>> +             * possible */
>>>> +            return num_x->u.i64 == num_y->u.dbl;
>>>
>>> Overflow is impossible, but loss of precision is possible:
>>>
>>>     (double)9007199254740993ull == 9007199254740992.0
>>>
>>> yields true.  Is this what we want?
>> 
>> I'd argue that yes, because the floating point value represents
>> basically all of the values which are "equal" to it.
>
> But the problem is that we CAN represent the fully-precise number as an
> integer, so having multiple distinct integers that compare differently
> against each other, but equal to the same double, is awkward.

Yup.

>> But I don't have a string opinion. I guess the alternative would be to
>> convert the double to an integer instead and check for overflows before?
>
> That's the solution Markus gave, and I'm in favor of the tighter check:
>
[...]
>>> I think there's more than one sane interpretations of "is equal",
>>> including:
>>> 
>>> * The mathematical numbers represented by @x and @y are equal.
>>> 
>>> * @x and @y have the same contents, i.e. same kind and u.
>>> 
>>> * @x and @y are the same object (listed for completeness; we don't need
>>>   a function to compare pointers).
>>> 
>>> Your patch implements yet another one.  Which one do we want, and why?
>>
>> Mine is the first one, except that I think that a floating point value
>> does not represent a single number but just some number in a range.
>>
>>> The second is easier to implement than the first.
>>
>> It seems much less useful, though.

Depends on what for.

Common Lisp has

* eq: same object
* eql: eq, or both numbers with same type and value, or both characters
  that represent the same character
* =: mathematically equal (arguments must be numbers)
* equal: like eql, but it recursively descends into lists (I'm
  simplifying)

Decades of use back the assertion that eq, eql and = are all useful.

>>> If we really want the first, you need to fix the loss of precision bugs.
>>
>> I'm not sure, but I don't mind either, so...

For what it's worth, your v2 had QInt compare not equal to QFloat.
Makes me suspect that eql is good enough for the problem at hand.

>>> I guess the obvious fix is
>>>
>>>     return (double)x == x && x == y;
>> 
>> Yes, that would do, too; and spares me of having to think about how well
>> comparing an arbitrary double to UINT64_MAX actually works. :-)
>
> It basically says that we are unwilling to declare an integer equivalent
> to the double if the double loses precision when trying to store the
> integer.
>
>>> Note that this is what you do for mixed signedness: first check @x is
>>> exactly representable in @y's type, then compare in @y's type.
>>> 
>>> Regardless of which one we pick, the function comment needs to explain.

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 15:48, Max Reitz wrote:
> On 2017-07-05 09:07, Markus Armbruster wrote:
>> 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>
>> [...]
>>> diff --git a/qobject/qnum.c b/qobject/qnum.c
>>> index 476e81c..784d061 100644
>>> --- a/qobject/qnum.c
>>> +++ b/qobject/qnum.c
>>> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>>>  }
>>>  
>>>  /**
>>> + * qnum_is_equal(): Test whether the two QNums are equal
>>> + */
>>> +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:
>>> +            /* Implicit conversion of x to double; no overflow
>>> +             * possible */
>>> +            return num_x->u.i64 == num_y->u.dbl;
>>
>> Overflow is impossible, but loss of precision is possible:
>>
>>     (double)9007199254740993ull == 9007199254740992.0
>>
>> yields true.  Is this what we want?
> 
> I'd argue that yes, because the floating point value represents
> basically all of the values which are "equal" to it.
> 
> But I don't have a string opinion. I guess the alternative would be to
> convert the double to an integer instead and check for overflows before?
> 
>>> +        }
>>> +        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:
>>> +            /* Implicit conversion of x to double; no overflow
>>> +             * possible */
>>> +            return num_x->u.u64 == num_y->u.dbl;
>>
>> Similar loss of precision.
>>
>>> +        }
>>> +        abort();
>>> +    case QNUM_DOUBLE:
>>> +        switch (num_y->kind) {
>>> +        case QNUM_I64:
>>> +            return qnum_is_equal(y, x);
>>> +        case QNUM_U64:
>>> +            return qnum_is_equal(y, x);
>>> +        case QNUM_DOUBLE:
>>> +            /* Comparison in native double type */
>>> +            return num_x->u.dbl == num_y->u.dbl;
>>> +        }
>>> +        abort();
>>> +    }
>>> +
>>> +    abort();
>>> +}
>>
>> I think there's more than one sane interpretations of "is equal",
>> including:
>>
>> * The mathematical numbers represented by @x and @y are equal.
>>
>> * @x and @y have the same contents, i.e. same kind and u.
>>
>> * @x and @y are the same object (listed for completeness; we don't need
>>   a function to compare pointers).
>>
>> Your patch implements yet another one.  Which one do we want, and why?
> 
> Mine is the first one, except that I think that a floating point value
> does not represent a single number but just some number in a range.
> 
>> The second is easier to implement than the first.
> 
> It seems much less useful, though.
> 
>> If we really want the first, you need to fix the loss of precision bugs.
> 
> I'm not sure, but I don't mind either, so...
> 
>> I guess the obvious fix is
>>
>>     return (double)x == x && x == y;
> 
> Yes, that would do, too; and spares me of having to think about how well
> comparing an arbitrary double to UINT64_MAX actually works. :-)

On second thought, this won't do, because (double)x == x is always true
if x is an integer (because this will implicitly cast the second x to a
double, too). However, (uint64_t)(double)x == x should work.

Max

> 
>> Note that this is what you do for mixed signedness: first check @x is
>> exactly representable in @y's type, then compare in @y's type.
>>
>> Regardless of which one we pick, the function comment needs to explain.
> 
> OK, will do.
> 
> Max
> 
>>> +
>>> +/**
>>>   * qnum_destroy_obj(): Free all memory allocated by a
>>>   * QNum object
>>>   */
>> [...]
>>
>> Remainder of the patch looks good to me.
>>
> 
> 



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

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 18:05, Max Reitz wrote:
> On 2017-07-05 15:48, Max Reitz wrote:
>> On 2017-07-05 09:07, Markus Armbruster wrote:
>>> 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>
>>> [...]
>>>> diff --git a/qobject/qnum.c b/qobject/qnum.c
>>>> index 476e81c..784d061 100644
>>>> --- a/qobject/qnum.c
>>>> +++ b/qobject/qnum.c
>>>> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>>>>  }
>>>>  
>>>>  /**
>>>> + * qnum_is_equal(): Test whether the two QNums are equal
>>>> + */
>>>> +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:
>>>> +            /* Implicit conversion of x to double; no overflow
>>>> +             * possible */
>>>> +            return num_x->u.i64 == num_y->u.dbl;
>>>
>>> Overflow is impossible, but loss of precision is possible:
>>>
>>>     (double)9007199254740993ull == 9007199254740992.0
>>>
>>> yields true.  Is this what we want?
>>
>> I'd argue that yes, because the floating point value represents
>> basically all of the values which are "equal" to it.
>>
>> But I don't have a string opinion. I guess the alternative would be to
>> convert the double to an integer instead and check for overflows before?
>>
>>>> +        }
>>>> +        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:
>>>> +            /* Implicit conversion of x to double; no overflow
>>>> +             * possible */
>>>> +            return num_x->u.u64 == num_y->u.dbl;
>>>
>>> Similar loss of precision.
>>>
>>>> +        }
>>>> +        abort();
>>>> +    case QNUM_DOUBLE:
>>>> +        switch (num_y->kind) {
>>>> +        case QNUM_I64:
>>>> +            return qnum_is_equal(y, x);
>>>> +        case QNUM_U64:
>>>> +            return qnum_is_equal(y, x);
>>>> +        case QNUM_DOUBLE:
>>>> +            /* Comparison in native double type */
>>>> +            return num_x->u.dbl == num_y->u.dbl;
>>>> +        }
>>>> +        abort();
>>>> +    }
>>>> +
>>>> +    abort();
>>>> +}
>>>
>>> I think there's more than one sane interpretations of "is equal",
>>> including:
>>>
>>> * The mathematical numbers represented by @x and @y are equal.
>>>
>>> * @x and @y have the same contents, i.e. same kind and u.
>>>
>>> * @x and @y are the same object (listed for completeness; we don't need
>>>   a function to compare pointers).
>>>
>>> Your patch implements yet another one.  Which one do we want, and why?
>>
>> Mine is the first one, except that I think that a floating point value
>> does not represent a single number but just some number in a range.
>>
>>> The second is easier to implement than the first.
>>
>> It seems much less useful, though.
>>
>>> If we really want the first, you need to fix the loss of precision bugs.
>>
>> I'm not sure, but I don't mind either, so...
>>
>>> I guess the obvious fix is
>>>
>>>     return (double)x == x && x == y;
>>
>> Yes, that would do, too; and spares me of having to think about how well
>> comparing an arbitrary double to UINT64_MAX actually works. :-)
> 
> On second thought, this won't do, because (double)x == x is always true
> if x is an integer (because this will implicitly cast the second x to a
> double, too). However, (uint64_t)(double)x == x should work.

Hm, well, the nice thing with this is that (double)UINT64_MAX is
actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is
undefined... Urgs.

So I guess one thing that isn't very obvious but that should *always*
work (and is always well-defined) is this:

For uint64_t: y < 0x1p64 && (uint64_t)y == x

For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x

I hope. :-/

(But finally a chance to use binary exponents! Yay!)

Max


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

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/05/2017 11:22 AM, Max Reitz wrote:

>>>>     return (double)x == x && x == y;
>>>
>>> Yes, that would do, too; and spares me of having to think about how well
>>> comparing an arbitrary double to UINT64_MAX actually works. :-)
>>
>> On second thought, this won't do, because (double)x == x is always true
>> if x is an integer (because this will implicitly cast the second x to a
>> double, too). However, (uint64_t)(double)x == x should work.
> 
> Hm, well, the nice thing with this is that (double)UINT64_MAX is
> actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is
> undefined... Urgs.

(uint64_t)(UINT64_MAX + 1) is well-defined - it is 0.

(Adding in unsigned integers is always well-defined - it wraps around on
mathematical overflow modulo the integer size.  You're thinking of
overflow addition on signed integers, which is indeed undefined)

> 
> So I guess one thing that isn't very obvious but that should *always*
> work (and is always well-defined) is this:
> 
> For uint64_t: y < 0x1p64 && (uint64_t)y == x
> 
> For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x

That's harder to read, compared to the double-cast method which is
well-defined after all.

> 
> I hope. :-/
> 
> (But finally a chance to use binary exponents! Yay!)

Justifying the use of binary exponents is going to be harder than that,
and would need a really good comment in the code, compared to just using
a safe double-cast.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 18:29, Eric Blake wrote:
> On 07/05/2017 11:22 AM, Max Reitz wrote:
> 
>>>>>     return (double)x == x && x == y;
>>>>
>>>> Yes, that would do, too; and spares me of having to think about how well
>>>> comparing an arbitrary double to UINT64_MAX actually works. :-)
>>>
>>> On second thought, this won't do, because (double)x == x is always true
>>> if x is an integer (because this will implicitly cast the second x to a
>>> double, too). However, (uint64_t)(double)x == x should work.
>>
>> Hm, well, the nice thing with this is that (double)UINT64_MAX is
>> actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is
>> undefined... Urgs.
> 
> (uint64_t)(UINT64_MAX + 1) is well-defined - it is 0.
> 
> (Adding in unsigned integers is always well-defined - it wraps around on
> mathematical overflow modulo the integer size.  You're thinking of
> overflow addition on signed integers, which is indeed undefined)

It's not. See the standard:

When a finite value of real floating type is converted to an integer
type other than _Bool, the fractional part is discarded (i.e., the value
is truncated toward zero). If the value of the integral part cannot be
represented by the integer type, the behavior is undefined. [61]

[61] The remaindering operation performed when a value of integer type
is converted to unsigned type need not be performed when a value of real
floating type is converted to unsigned type. Thus, the range of portable
real floating values is (−1, Utype_MAX+1).

See for yourself:

printf("%i\n", (uint64_t)(double)UINT64_MAX == UINT64_MAX);

This yields 1 with gcc and -O3.

>>
>> So I guess one thing that isn't very obvious but that should *always*
>> work (and is always well-defined) is this:
>>
>> For uint64_t: y < 0x1p64 && (uint64_t)y == x
>>
>> For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x
> 
> That's harder to read, compared to the double-cast method which is
> well-defined after all.
> 
>>
>> I hope. :-/
>>
>> (But finally a chance to use binary exponents! Yay!)
> 
> Justifying the use of binary exponents is going to be harder than that,
> and would need a really good comment in the code, compared to just using
> a safe double-cast.

It's not safe.

Max


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

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 19:00, Max Reitz wrote:
> On 2017-07-05 18:29, Eric Blake wrote:
>> On 07/05/2017 11:22 AM, Max Reitz wrote:
>>
>>>>>>     return (double)x == x && x == y;
>>>>>
>>>>> Yes, that would do, too; and spares me of having to think about how well
>>>>> comparing an arbitrary double to UINT64_MAX actually works. :-)
>>>>
>>>> On second thought, this won't do, because (double)x == x is always true
>>>> if x is an integer (because this will implicitly cast the second x to a
>>>> double, too). However, (uint64_t)(double)x == x should work.
>>>
>>> Hm, well, the nice thing with this is that (double)UINT64_MAX is
>>> actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is
>>> undefined... Urgs.
>>
>> (uint64_t)(UINT64_MAX + 1) is well-defined - it is 0.
>>
>> (Adding in unsigned integers is always well-defined - it wraps around on
>> mathematical overflow modulo the integer size.  You're thinking of
>> overflow addition on signed integers, which is indeed undefined)
> 
> It's not. See the standard:

Or, well, yes, it is in this case, but I meant literally UINT64_MAX + 1,
not the uint64_t value. I meant the natural number 2^64.

Because the issue is that (double)UINT64_MAX will (or may, depending on
the environment and such) give us 2.0^64 == 0x1p64.

And this is where the quotation I gave below comes in. When casting an
integer to a double we may end up with a value that is not representable
in the original integer type, so we cannot easily cast it back.

Max

> When a finite value of real floating type is converted to an integer
> type other than _Bool, the fractional part is discarded (i.e., the value
> is truncated toward zero). If the value of the integral part cannot be
> represented by the integer type, the behavior is undefined. [61]
> 
> [61] The remaindering operation performed when a value of integer type
> is converted to unsigned type need not be performed when a value of real
> floating type is converted to unsigned type. Thus, the range of portable
> real floating values is (−1, Utype_MAX+1).
> 
> See for yourself:
> 
> printf("%i\n", (uint64_t)(double)UINT64_MAX == UINT64_MAX);
> 
> This yields 1 with gcc and -O3.
> 
>>>
>>> So I guess one thing that isn't very obvious but that should *always*
>>> work (and is always well-defined) is this:
>>>
>>> For uint64_t: y < 0x1p64 && (uint64_t)y == x
>>>
>>> For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x
>>
>> That's harder to read, compared to the double-cast method which is
>> well-defined after all.
>>
>>>
>>> I hope. :-/
>>>
>>> (But finally a chance to use binary exponents! Yay!)
>>
>> Justifying the use of binary exponents is going to be harder than that,
>> and would need a really good comment in the code, compared to just using
>> a safe double-cast.
> 
> It's not safe.
> 
> Max
> 



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

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/05/2017 12:04 PM, Max Reitz wrote:

> Or, well, yes, it is in this case, but I meant literally UINT64_MAX + 1,
> not the uint64_t value. I meant the natural number 2^64.
> 
> Because the issue is that (double)UINT64_MAX will (or may, depending on
> the environment and such) give us 2.0^64 == 0x1p64.
> 
> And this is where the quotation I gave below comes in. When casting an
> integer to a double we may end up with a value that is not representable
> in the original integer type, so we cannot easily cast it back.

And it's more than just UINT64_MAX that rounds to the double value of
0x1p64.  Okay, I'm starting to be convinced that using a floating-point
comparison to 0x1p64 is going to be easier than an integer comparison to
the rounding point (what point is that, anyways: where the low-order 11
bits that don't fit in 53 bits of precision cause us to round up instead
of down?)

>>> Justifying the use of binary exponents is going to be harder than that,
>>> and would need a really good comment in the code, compared to just using
>>> a safe double-cast.
>>
>> It's not safe.
>>
>> Max
>>
> 
> 

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/05/2017 12:00 PM, Max Reitz wrote:
>> (uint64_t)(UINT64_MAX + 1) is well-defined - it is 0.
>>
>> (Adding in unsigned integers is always well-defined - it wraps around on
>> mathematical overflow modulo the integer size.  You're thinking of
>> overflow addition on signed integers, which is indeed undefined)
> 
> It's not. See the standard:
> 
> When a finite value of real floating type is converted to an integer

Ah, you're talking:

(uint64_t)(double)(UINT64_MAX + 1), which is indeed undefined.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-07-05 18:22, Max Reitz wrote:
> On 2017-07-05 18:05, Max Reitz wrote:
>> On 2017-07-05 15:48, Max Reitz wrote:
>>> On 2017-07-05 09:07, Markus Armbruster wrote:
>>>> 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>
>>>> [...]
>>>>> diff --git a/qobject/qnum.c b/qobject/qnum.c
>>>>> index 476e81c..784d061 100644
>>>>> --- a/qobject/qnum.c
>>>>> +++ b/qobject/qnum.c
>>>>> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>>>>>  }
>>>>>  
>>>>>  /**
>>>>> + * qnum_is_equal(): Test whether the two QNums are equal
>>>>> + */
>>>>> +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:
>>>>> +            /* Implicit conversion of x to double; no overflow
>>>>> +             * possible */
>>>>> +            return num_x->u.i64 == num_y->u.dbl;
>>>>
>>>> Overflow is impossible, but loss of precision is possible:
>>>>
>>>>     (double)9007199254740993ull == 9007199254740992.0
>>>>
>>>> yields true.  Is this what we want?
>>>
>>> I'd argue that yes, because the floating point value represents
>>> basically all of the values which are "equal" to it.
>>>
>>> But I don't have a string opinion. I guess the alternative would be to
>>> convert the double to an integer instead and check for overflows before?
>>>
>>>>> +        }
>>>>> +        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:
>>>>> +            /* Implicit conversion of x to double; no overflow
>>>>> +             * possible */
>>>>> +            return num_x->u.u64 == num_y->u.dbl;
>>>>
>>>> Similar loss of precision.
>>>>
>>>>> +        }
>>>>> +        abort();
>>>>> +    case QNUM_DOUBLE:
>>>>> +        switch (num_y->kind) {
>>>>> +        case QNUM_I64:
>>>>> +            return qnum_is_equal(y, x);
>>>>> +        case QNUM_U64:
>>>>> +            return qnum_is_equal(y, x);
>>>>> +        case QNUM_DOUBLE:
>>>>> +            /* Comparison in native double type */
>>>>> +            return num_x->u.dbl == num_y->u.dbl;
>>>>> +        }
>>>>> +        abort();
>>>>> +    }
>>>>> +
>>>>> +    abort();
>>>>> +}
>>>>
>>>> I think there's more than one sane interpretations of "is equal",
>>>> including:
>>>>
>>>> * The mathematical numbers represented by @x and @y are equal.
>>>>
>>>> * @x and @y have the same contents, i.e. same kind and u.
>>>>
>>>> * @x and @y are the same object (listed for completeness; we don't need
>>>>   a function to compare pointers).
>>>>
>>>> Your patch implements yet another one.  Which one do we want, and why?
>>>
>>> Mine is the first one, except that I think that a floating point value
>>> does not represent a single number but just some number in a range.
>>>
>>>> The second is easier to implement than the first.
>>>
>>> It seems much less useful, though.
>>>
>>>> If we really want the first, you need to fix the loss of precision bugs.
>>>
>>> I'm not sure, but I don't mind either, so...
>>>
>>>> I guess the obvious fix is
>>>>
>>>>     return (double)x == x && x == y;
>>>
>>> Yes, that would do, too; and spares me of having to think about how well
>>> comparing an arbitrary double to UINT64_MAX actually works. :-)
>>
>> On second thought, this won't do, because (double)x == x is always true
>> if x is an integer (because this will implicitly cast the second x to a
>> double, too). However, (uint64_t)(double)x == x should work.
> 
> Hm, well, the nice thing with this is that (double)UINT64_MAX is
> actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is
> undefined... Urgs.
> 
> So I guess one thing that isn't very obvious but that should *always*
> work (and is always well-defined) is this:
> 
> For uint64_t: y < 0x1p64 && (uint64_t)y == x

Here comes iteration number 4: Forgot the y >= 0 check.

> For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x

Also, I should check that the fractional part of y is 0 (through modf(y,
&_) == 0.0).

Floating point numbers are so much fun!

(And all of this gives me such great ideas for tests to add to patch 5!)

Max

> I hope. :-/
> 
> (But finally a chance to use binary exponents! Yay!)
> 
> Max
> 



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

[Qemu-devel] [PATCH v3 3/5] 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).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 913bb43..45eb248 100644
--- a/block.c
+++ b/block.c
@@ -2947,19 +2947,24 @@ 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);
-            /*
-             * 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.
-             */
-            const char *old = qdict_get_try_str(reopen_state->bs->options,
-                                                entry->key);
-
-            if (!old || strcmp(new, old)) {
+            QObject *new = entry->value;
+            QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+
+            /* 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 simply can not specify options which cannot be changed
+             * anyway, so they will stay unchanged. */
+            if (!qobject_is_equal(new, old)) {
                 error_setg(errp, "Cannot change the option '%s'", entry->key);
                 ret = -EINVAL;
                 goto error;
-- 
2.9.4

Re: [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare()

[Eric Blake]

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

On 07/03/2017 07:25 AM, Max Reitz wrote:
> 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

s/invokable/invocable/

> 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).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 

> +            /* 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 simply can not specify options which cannot be changed

Seeing "can not" usually looks wrong for "cannot"; but here, it is
grammatically correct.  But better would be "the user can simply not
specify" or "the user can simply avoid specifying options".

> +             * anyway, so they will stay unchanged. */

The grammar tweak is minor, so:
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

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

On 2017-07-03 14:51, Eric Blake wrote:
> On 07/03/2017 07:25 AM, Max Reitz wrote:
>> 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
> 
> s/invokable/invocable/

I was asking this myself when writing this commit message and actually
consulted Wiktionary: https://en.wiktionary.org/wiki/invokable

>> 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).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 31 ++++++++++++++++++-------------
>>  1 file changed, 18 insertions(+), 13 deletions(-)
>>
> 
>> +            /* 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 simply can not specify options which cannot be changed
> 
> Seeing "can not" usually looks wrong for "cannot"; but here, it is
> grammatically correct.  But better would be "the user can simply not
> specify" or "the user can simply avoid specifying options".

Awww, I deliberately designed the sentence so I could use "can not". :-)

(and even contrast it with the following "cannot"!)

>> +             * anyway, so they will stay unchanged. */
> 
> The grammar tweak is minor, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>
Please let me keep it :-)

Max


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

Re: [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare()

[Eric Blake]

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

On 07/03/2017 08:01 AM, Max Reitz wrote:
> On 2017-07-03 14:51, Eric Blake wrote:
>> On 07/03/2017 07:25 AM, Max Reitz wrote:
>>> 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
>>
>> s/invokable/invocable/
> 
> I was asking this myself when writing this commit message and actually
> consulted Wiktionary: https://en.wiktionary.org/wiki/invokable

My intuitive reasoning for favoring 'c' is that we spell it
"invocation", not "invokation", even if the short form is "invoke".

But I also consulted dictionary.com before writing my original reply:
http://www.dictionary.com/browse/invocable
which pulls up "invoke" as the primary, and only lists "invocable" (and
not "invokable") as the related form adjective.

Meanwhile, you are correct that wiktionary does list both forms as
alternates of one another, so I can live with the 'k' even though it
looks odd.


>>> +             * all options.  For now, this is not too big of an issue because
>>> +             * the user simply can not specify options which cannot be changed
>>
>> Seeing "can not" usually looks wrong for "cannot"; but here, it is
>> grammatically correct.  But better would be "the user can simply not
>> specify" or "the user can simply avoid specifying options".
> 
> Awww, I deliberately designed the sentence so I could use "can not". :-)
> 
> (and even contrast it with the following "cannot"!)

Maybe it's just that I'm being thrown off by the placement of 'simply';
as a native speaker, I have an easier time parsing "can simply not" than
I do "simply can not", maybe because of where the emphasis of "simply"
is falling.  Which is perhaps weird, because in the infinitive form
(coming up with a construct that uses "to" instead of "can"), I note
that "it is possible to simply not specify options" is a split
infinitive (which style guides tell you to avoid, even though I think it
is acceptable); while "it is possible simply to not specify options"
satisfies more style guides.

But since you are intentionally doing it for the play on words,

> 
>>> +             * anyway, so they will stay unchanged. */
>>
>> The grammar tweak is minor, so:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Please let me keep it :-)

you can keep it.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare()

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> 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).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/block.c b/block.c
> index 913bb43..45eb248 100644
> --- a/block.c
> +++ b/block.c
> @@ -2947,19 +2947,24 @@ 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);
> -            /*
> -             * 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.
> -             */
> -            const char *old = qdict_get_try_str(reopen_state->bs->options,
> -                                                entry->key);
> -
> -            if (!old || strcmp(new, old)) {
> +            QObject *new = entry->value;
> +            QObject *old = qdict_get(reopen_state->bs->options, entry->key);
> +
> +            /* 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 simply can not specify options which cannot be changed
> +             * anyway, so they will stay unchanged. */

I'm not the maintainer, and this is not a demand: consider winging this
comment and wrapping lines around column 70.

As much as I fancy word play (see your reply to Eric), I have to admit
that I had to read "the user simply can not specify options" about three
times to make sense of it.  Please consider a wording that's easier to
grasp, perhaps "the user can simply refrain from specifying options that
cannot be changed".

> +            if (!qobject_is_equal(new, old)) {
>                  error_setg(errp, "Cannot change the option '%s'", entry->key);
>                  ret = -EINVAL;
>                  goto error;

Re: [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

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

On 2017-07-05 09:14, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> 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).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 31 ++++++++++++++++++-------------
>>  1 file changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 913bb43..45eb248 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2947,19 +2947,24 @@ 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);
>> -            /*
>> -             * 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.
>> -             */
>> -            const char *old = qdict_get_try_str(reopen_state->bs->options,
>> -                                                entry->key);
>> -
>> -            if (!old || strcmp(new, old)) {
>> +            QObject *new = entry->value;
>> +            QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>> +
>> +            /* 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 simply can not specify options which cannot be changed
>> +             * anyway, so they will stay unchanged. */
> 
> I'm not the maintainer, and this is not a demand: consider winging this
> comment and wrapping lines around column 70.

I actually did consider wrapping around column 70 and decided against it
because this comment is already indented by 12 characters, so none of
the lines exceed 65 characters (80 - (12 + strlen(" * "))).

About winging...  For some reason I don't quite like it here.  But it
probably is better because the comment is not immediately related to the
qobject_is_equal() call below, so I'll do it.

> As much as I fancy word play (see your reply to Eric), I have to admit
> that I had to read "the user simply can not specify options" about three
> times to make sense of it.  Please consider a wording that's easier to
> grasp, perhaps "the user can simply refrain from specifying options that
> cannot be changed".

Aw.  I've wanted this to be an example I could point people to to
educate them about the difference.  Now, alas, none shall learn. :-(

Max


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

[Qemu-devel] [PATCH v3 4/5] 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 9d35a6a..af6b3e1 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 cc86b94..f4a85ae 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.9.4

[Qemu-devel] [PATCH v3 5/5] 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  | 312 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 315 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qobject.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index c738e92..ff0022d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -18,6 +18,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/test-qobject-output-visitor$(EXESUF)
@@ -507,7 +508,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/test-coroutine.o tests/test-string-output-visitor.o \
 	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
@@ -540,6 +541,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-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
 tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
new file mode 100644
index 0000000..a68cb1e
--- /dev/null
+++ b/tests/check-qobject.c
@@ -0,0 +1,312 @@
+/*
+ * 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"
+
+/* 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).
+ */
+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, *d0p25, *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);
+    d0p25 = qnum_from_double(0.25);
+    dnan = qnum_from_double(0.0 / 0.0);
+    um42 = qnum_from_uint((uint64_t)-42);
+    im42 = qnum_from_int(-42);
+    dm42 = qnum_from_int(-42.0);
+
+    s0 = qstring_from_str("0");
+    s_empty = qstring_new();
+    bfalse = qbool_from_bool(false);
+
+    /* The internal representation should not matter, as long as the
+     * precision is sufficient */
+    test_equality(true, u0, 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 round */
+    test_equality(false, u0, d0p25);
+    test_equality(false, i0, d0p25);
+
+    /* Do not assume any object is equal to itself */
+    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
+
+    /* No unsigned overflow */
+    test_equality(false, um42, im42);
+    test_equality(false, um42, dm42);
+    test_equality(true, im42, dm42);
+
+    free_all(u0, i0, d0, d0p25, 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_obj(list_longer, qnull());
+
+    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(0.0 / 0.0));
+    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_obj(dict_0, "null", qnull());
+
+    qdict_put_int(dict_1, "f.o", 1);
+    qdict_put_int(dict_1, "bar", 2);
+    qdict_put_int(dict_1, "baz", 3);
+    qdict_put_obj(dict_1, "null", qnull());
+
+    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_obj(dict_different_key, "null", qnull());
+
+    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_obj(dict_different_value, "null", qnull());
+
+    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_obj(dict_different_null_key, "none", qnull());
+
+    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_obj(dict_longer, "null", qnull());
+
+    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_obj(dict_nested, "null", qnull());
+
+    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(0.0 / 0.0));
+    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.9.4

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add check-qobject for equality tests

[Eric Blake]

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

On 07/03/2017 07:25 AM, 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  | 312 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 315 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qobject.c
> 

> + * 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).

May be worth expanding the comment to mention NaN in QNum as the culprit
for this fact.


> +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) {

Here, you're careful to allow a NULL argument,


> +static void do_free_all(int _, ...)
> +{
> +    va_list ap;
> +    QObject *obj;
> +
> +    va_start(ap, _);
> +    while ((obj = va_arg(ap, QObject *)) != NULL) {
> +        qobject_decref(obj);
> +    }

Here, you stop on the first NULL, and aren't checking for the special
sentinel that can't be freed.

> +    va_end(ap);
> +}
> +
> +#define free_all(...) \
> +    do_free_all(0, __VA_ARGS__, NULL)

Then again, you don't pass the special sentinel. So as long as NULL is
the last argument(s) on any test that passes NULL (rather than an
intermediate argument), you don't need to use the sentinel, and stopping
iteration on the first NULL is okay.  A bit magic, but I can live with it.

> +
> +static void qobject_is_equal_null_test(void)
> +{
> +    test_equality(false, qnull(), NULL);
> +}

Where do you test_equality(true, NULL, NULL) ?

> +
> +static void qobject_is_equal_num_test(void)
> +{
> +    QNum *u0, *i0, *d0, *d0p25, *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);
> +    d0p25 = qnum_from_double(0.25);
> +    dnan = qnum_from_double(0.0 / 0.0);

Ah, so you ARE testing NaN as a QNum, even though it can't occur in
JSON.  Might be worth a comment.


> +
> +    /* Containing an NaN value will make this dict compare unequal to

s/an/a/ (if you pronounce it "nan" or "not-a-number") (but I guess no
change if you pronounce it "en-a-en")

There might be some improvements to add, but as written the test is
reasonable, and some testing is better than none, so:
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add check-qobject for equality tests

[Max Reitz]

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

On 2017-07-03 16:15, Eric Blake wrote:
> On 07/03/2017 07:25 AM, 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  | 312 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 315 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/check-qobject.c
>>
> 
>> + * 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).
> 
> May be worth expanding the comment to mention NaN in QNum as the culprit
> for this fact.

True.

>> +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) {
> 
> Here, you're careful to allow a NULL argument,
> 
> 
>> +static void do_free_all(int _, ...)
>> +{
>> +    va_list ap;
>> +    QObject *obj;
>> +
>> +    va_start(ap, _);
>> +    while ((obj = va_arg(ap, QObject *)) != NULL) {
>> +        qobject_decref(obj);
>> +    }
> 
> Here, you stop on the first NULL, and aren't checking for the special
> sentinel that can't be freed.
> 
>> +    va_end(ap);
>> +}
>> +
>> +#define free_all(...) \
>> +    do_free_all(0, __VA_ARGS__, NULL)
> 
> Then again, you don't pass the special sentinel. So as long as NULL is
> the last argument(s) on any test that passes NULL (rather than an
> intermediate argument), you don't need to use the sentinel, and stopping
> iteration on the first NULL is okay.  A bit magic, but I can live with it.

I don't mind using the sentinel here, too, but passing NULL doesn't make
much sense here. It does for test_equality(), though.

>> +
>> +static void qobject_is_equal_null_test(void)
>> +{
>> +    test_equality(false, qnull(), NULL);
>> +}
> 
> Where do you test_equality(true, NULL, NULL) ?

Automatically in do_test_equality().

(It automatically tests whether all arguments are equal to itself --
which is why I can't use it to test NaN.)

>> +
>> +static void qobject_is_equal_num_test(void)
>> +{
>> +    QNum *u0, *i0, *d0, *d0p25, *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);
>> +    d0p25 = qnum_from_double(0.25);
>> +    dnan = qnum_from_double(0.0 / 0.0);
> 
> Ah, so you ARE testing NaN as a QNum, even though it can't occur in
> JSON.  Might be worth a comment.

Sure, why not.

>> +
>> +    /* Containing an NaN value will make this dict compare unequal to
> 
> s/an/a/ (if you pronounce it "nan" or "not-a-number") (but I guess no
> change if you pronounce it "en-a-en")

I pronounce it en-a-en, but I don't know whether that's the usual way. ;-)

> There might be some improvements to add, but as written the test is
> reasonable, and some testing is better than none, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


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

Re: [Qemu-devel] [PATCH v3 5/5] 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>

I'll review this one as soon as we made up our minds on what exactly
qnum_is_equal() should do.