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

[Qemu-devel] [PATCH v4 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.


v4:
- Patch 1: Kept, because Markus gave his R-b already...
- Patch 2: Check that doubles match integers exactly, not just after the
           integer has been (lossily) converted to a double
           [Markus/Eric]
- Patch 3: Winged comment and s/simply can not/can simply omit/ (:-()
           [Markus/Eric]
- Patch 5:
  - Mention (at least one) reason for the non-reflexivity of
    qobject_is_equal() (that being NaN) [Eric]
  - Mention that NaN cannot occur in JSON [Eric]
  - Tests for integer values that cannot be exactly represented as a
    double


git-backport-diff against v3:

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:[----] [--] 'qapi/qnull: Add own header'
002/5:[0032] [FC] 'qapi: Add qobject_is_equal()'
003/5:[0022] [FC] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
004/5:[----] [--] 'iotests: Add test for non-string option reopening'
005/5:[0114] [FC] '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                    |  29 ++--
 qobject/qbool.c            |   8 +
 qobject/qdict.c            |  29 ++++
 qobject/qlist.c            |  32 ++++
 qobject/qnull.c            |  11 +-
 qobject/qnum.c             |  77 +++++++++
 qobject/qobject.c          |  29 ++++
 qobject/qstring.c          |   9 +
 tests/check-qnull.c        |   2 +-
 tests/check-qobject.c      | 404 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/133     |   9 +
 tests/qemu-iotests/133.out |   5 +
 21 files changed, 677 insertions(+), 22 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.9.4

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

[Max Reitz]

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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

[Qemu-devel] [PATCH v4 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>
---
Markus also proposed just reporting two values as unequal if they have a
different internal representation (i.e. a different QNum kind).

I don't like this very much, because I feel like QInt and QFloat have
been unified for a reason: Outside of these classes, nobody should care
about the exact internal representation.  In JSON, there is no
difference anyway.  We probably want to use integers as long as we can
and doubles whenever we cannot.

In any case, I feel like the class should hide the different internal
representations from the user.  This necessitates being able to compare
floating point values against integers.  Since apparently the main use
of QObject is to parse and emit JSON (and represent such objects
internally), we also have to agree that JSON doesn't make a difference:
42 is just the same as 42.0.

Finally, I think it's rather pointless not to consider 42u and 42 the
same value.  But since unsigned/signed are two different kinds of QNums
already, we cannot consider them equal without considering 42.0 equal,
too.

Because of this, I have decided to continue to compare QNum values even
if they are of a different kind.
---
 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             | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 qobject/qobject.c          | 29 ++++++++++++++++++
 qobject/qstring.c          |  9 ++++++
 14 files changed, 205 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..96c348c 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -18,6 +18,8 @@
 #include "qapi/qmp/qobject.h"
 #include "qemu-common.h"
 
+#include <math.h>
+
 /**
  * qnum_from_int(): Create a new QNum from an int64_t
  *
@@ -213,6 +215,77 @@ QNum *qobject_to_qnum(const QObject *obj)
 }
 
 /**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ *
+ * Negative integers are never considered equal to unsigned integers.
+ * Doubles are only considered equal to integers if their fractional
+ * part is zero and their integral part is exactly equal to the
+ * integer.  Because doubles have limited precision, there are
+ * therefore integers which do not have an equal double (e.g.
+ * INT64_MAX).
+ */
+bool qnum_is_equal(const QObject *x, const QObject *y)
+{
+    QNum *num_x = qobject_to_qnum(x);
+    QNum *num_y = qobject_to_qnum(y);
+    double integral_part; /* Needed for the modf() calls below */
+
+    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:
+            /* Comparing x to y in double (which the implicit
+             * conversion would do) is not exact.  So after having
+             * checked that y is an integer in the int64_t range
+             * (i.e. that it is within bounds and its fractional part
+             * is zero), compare both as integers. */
+            return num_y->u.dbl >= -0x1p63 && num_y->u.dbl < 0x1p63 &&
+                modf(num_y->u.dbl, &integral_part) == 0.0 &&
+                num_x->u.i64 == (int64_t)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:
+            /* Comparing x to y in double (which the implicit
+             * conversion would do) is not exact.  So after having
+             * checked that y is an integer in the uint64_t range
+             * (i.e. that it is within bounds and its fractional part
+             * is zero), compare both as integers. */
+            return num_y->u.dbl >= 0 && num_y->u.dbl < 0x1p64 &&
+                modf(num_y->u.dbl, &integral_part) == 0.0 &&
+                num_x->u.u64 == (uint64_t)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

[Qemu-devel] [PATCH v4 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 | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

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

[Qemu-devel] [PATCH v4 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 v4 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 +-
 qobject/qnum.c         |  16 +-
 tests/check-qobject.c  | 404 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 417 insertions(+), 7 deletions(-)
 create mode 100644 tests/check-qobject.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 42e17e2..07b130c 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)
@@ -508,7 +509,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 \
@@ -541,6 +542,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/qobject/qnum.c b/qobject/qnum.c
index 96c348c..3d029f6 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -217,12 +217,16 @@ QNum *qobject_to_qnum(const QObject *obj)
 /**
  * qnum_is_equal(): Test whether the two QNums are equal
  *
- * Negative integers are never considered equal to unsigned integers.
- * Doubles are only considered equal to integers if their fractional
- * part is zero and their integral part is exactly equal to the
- * integer.  Because doubles have limited precision, there are
- * therefore integers which do not have an equal double (e.g.
- * INT64_MAX).
+ * This comparison is done independently of the internal
+ * representation.  Any two numbers are considered equal if they are
+ * mathmatically equal, that means:
+ * - Negative integers are never considered equal to unsigned
+ *   integers.
+ * - Floating point values are only considered equal to integers if
+ *   their fractional part is zero and their integral part is exactly
+ *   equal to the integer.  Because doubles have limited precision,
+ *   there are therefore integers which do not have an equal floating
+ *   point value (e.g. INT64_MAX).
  */
 bool qnum_is_equal(const QObject *x, const QObject *y)
 {
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
new file mode 100644
index 0000000..fd964bf
--- /dev/null
+++ b/tests/check-qobject.c
@@ -0,0 +1,404 @@
+/*
+ * 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, e.g. in the case of a QNum containing NaN).
+ */
+static void do_test_equality(bool expected, ...)
+{
+    va_list ap_count, ap_extract;
+    QObject **args;
+    int arg_count = 0;
+    int i, j;
+
+    va_start(ap_count, expected);
+    va_copy(ap_extract, ap_count);
+    while (va_arg(ap_count, QObject *) != &_test_equality_end_of_arguments) {
+        arg_count++;
+    }
+    va_end(ap_count);
+
+    args = g_new(QObject *, arg_count);
+    for (i = 0; i < arg_count; i++) {
+        args[i] = va_arg(ap_extract, QObject *);
+    }
+    va_end(ap_extract);
+
+    for (i = 0; i < arg_count; i++) {
+        g_assert(qobject_is_equal(args[i], args[i]) == true);
+
+        for (j = i + 1; j < arg_count; j++) {
+            g_assert(qobject_is_equal(args[i], args[j]) == expected);
+        }
+    }
+}
+
+#define test_equality(expected, ...) \
+    do_test_equality(expected, __VA_ARGS__, &_test_equality_end_of_arguments)
+
+static void do_free_all(int _, ...)
+{
+    va_list ap;
+    QObject *obj;
+
+    va_start(ap, _);
+    while ((obj = va_arg(ap, QObject *)) != NULL) {
+        qobject_decref(obj);
+    }
+    va_end(ap);
+}
+
+#define free_all(...) \
+    do_free_all(0, __VA_ARGS__, NULL)
+
+static void qobject_is_equal_null_test(void)
+{
+    test_equality(false, qnull(), NULL);
+}
+
+static void qobject_is_equal_num_test(void)
+{
+    QNum *u0, *i0, *d0, *d0p25, *dnan, *um42, *im42, *dm42;
+    QNum *umax, *imax, *umax_exact, *umax_exact_p1;
+    QNum *dumax, *dimax, *dumax_exact, *dumax_exact_p1;
+    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);
+
+    /* 2^64 - 1: Not exactly representable as a double (needs 64 bits
+     * of precision, but double only has 53).  The double equivalent
+     * may be either 2^64 or 2^64 - 2^11. */
+    umax = qnum_from_uint(UINT64_MAX);
+
+    /* 2^63 - 1: Not exactly representable as a double (needs 63 bits
+     * of precision, but double only has 53).  The double equivalent
+     * may be either 2^63 or 2^63 - 2^10. */
+    imax = qnum_from_int(INT64_MAX);
+    /* 2^64 - 2^11: Exactly representable as a double (the least
+     * significant 11 bits are set to 0, so we only need the 53 bits
+     * of precision double offers).  This is the maximum value which
+     * is exactly representable both as a uint64_t and a double. */
+    umax_exact = qnum_from_uint(UINT64_MAX - 0x7ff);
+
+    /* 2^64 - 2^11 + 1: Not exactly representable as a double (needs
+     * 64 bits again), but whereas (double)UINT64_MAX may be rounded
+     * up to 2^64, this will most likely be rounded down to
+     * 2^64 - 2^11. */
+    umax_exact_p1 = qnum_from_uint(UINT64_MAX - 0x7ff + 1);
+
+    dumax = qnum_from_double((double)qnum_get_uint(umax));
+    dimax = qnum_from_double((double)qnum_get_int(imax));
+    dumax_exact = qnum_from_double((double)qnum_get_uint(umax_exact));
+    dumax_exact_p1 = qnum_from_double((double)qnum_get_uint(umax_exact_p1));
+
+    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 -- note however
+     * that NaN cannot occur in a JSON object anyway. */
+    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
+
+    /* No unsigned overflow */
+    test_equality(false, um42, im42);
+    test_equality(false, um42, dm42);
+    test_equality(true, im42, dm42);
+
+
+    /*
+     * Floating point values must match integers exactly to be
+     * considered equal; it does not suffice that converting the
+     * integer to a double yields the same value.
+     * Each of the following four tests follows the same pattern:
+     * 1. Check that both QNum objects compare unequal because they
+     *    are (mathematically).  The third test is an exception,
+     *    because here they are indeed equal.
+     * 2. Check that when converting the integer QNum to a double,
+     *    that value is equal to the double QNum.  We can thus see
+     *    that the QNum comparison does not simply convert the
+     *    integer to a floating point value (in a potentially lossy
+     *    operation).
+     * 3. Sanity checks: Check that the double QNum has the expected
+     *    value (which may be one of two in case it was rounded; the
+     *    exact result is then implementation-defined).
+     *    If there are multiple valid values, check that they are
+     *    distinct values when represented as double (just proving
+     *    that our assumptions about the precision of doubles are
+     *    correct).
+     *
+     * The first two tests are interesting because they may involve a
+     * double value which is out of the uint64_t or int64_t range,
+     * respectively (if it is rounded to 2^64 or 2^63 during
+     * conversion).
+     *
+     * Since both are intended to involve rounding the value up during
+     * conversion, we also have the fourth test which is indended to
+     * test behavior if the value was rounded down. This is the fourth
+     * test.
+     *
+     * The third test simply proves that the value used in the fourth
+     * test is indeed just one above a number that can be exactly
+     * represented in a double.
+     */
+
+    test_equality(false, umax, dumax);
+    g_assert(qnum_get_double(umax) == qnum_get_double(dumax));
+    g_assert(qnum_get_double(dumax) == 0x1p64 ||
+             qnum_get_double(dumax) == 0x1p64 - 0x1p11);
+    g_assert(0x1p64 != 0x1p64 - 0x1p11);
+
+    test_equality(false, imax, dimax);
+    g_assert(qnum_get_double(imax) == qnum_get_double(dimax));
+    g_assert(qnum_get_double(dimax) == 0x1p63 ||
+             qnum_get_double(dimax) == 0x1p63 - 0x1p10);
+    g_assert(0x1p63 != 0x1p63 - 0x1p10);
+
+    test_equality(true, umax_exact, dumax_exact);
+    g_assert(qnum_get_double(umax_exact) == qnum_get_double(dumax_exact));
+    g_assert(qnum_get_double(dumax_exact) == 0x1p64 - 0x1p11);
+
+    test_equality(false, umax_exact_p1, dumax_exact_p1);
+    g_assert(qnum_get_double(umax_exact_p1) == qnum_get_double(dumax_exact_p1));
+    g_assert(qnum_get_double(dumax_exact_p1) == 0x1p64 ||
+             qnum_get_double(dumax_exact_p1) == 0x1p64 - 0x1p11);
+    g_assert(0x1p64 != 0x1p64 - 0x1p11);
+
+
+    free_all(u0, i0, d0, d0p25, dnan, um42, im42, dm42,
+             umax, imax, umax_exact, umax_exact_p1,
+             dumax, dimax, dumax_exact, dumax_exact_p1,
+             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 v4 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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

On 07/05/2017 02:04 PM, Max Reitz wrote:
> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> Markus also proposed just reporting two values as unequal if they have a
> different internal representation (i.e. a different QNum kind).
> 
> I don't like this very much, because I feel like QInt and QFloat have
> been unified for a reason: Outside of these classes, nobody should care
> about the exact internal representation.  In JSON, there is no
> difference anyway.  We probably want to use integers as long as we can
> and doubles whenever we cannot.
> 
> In any case, I feel like the class should hide the different internal
> representations from the user.  This necessitates being able to compare
> floating point values against integers.  Since apparently the main use
> of QObject is to parse and emit JSON (and represent such objects
> internally), we also have to agree that JSON doesn't make a difference:
> 42 is just the same as 42.0.
> 
> Finally, I think it's rather pointless not to consider 42u and 42 the
> same value.  But since unsigned/signed are two different kinds of QNums
> already, we cannot consider them equal without considering 42.0 equal,
> too.
> 
> Because of this, I have decided to continue to compare QNum values even
> if they are of a different kind.

This explanation may deserve to be in the commit log proper.

>  /**
> + * qnum_is_equal(): Test whether the two QNums are equal
> + *
> + * Negative integers are never considered equal to unsigned integers.
> + * Doubles are only considered equal to integers if their fractional
> + * part is zero and their integral part is exactly equal to the
> + * integer.  Because doubles have limited precision, there are
> + * therefore integers which do not have an equal double (e.g.
> + * INT64_MAX).
> + */
> +bool qnum_is_equal(const QObject *x, const QObject *y)
> +{
> +    QNum *num_x = qobject_to_qnum(x);
> +    QNum *num_y = qobject_to_qnum(y);
> +    double integral_part; /* Needed for the modf() calls below */
> +
> +    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:
> +            /* Comparing x to y in double (which the implicit
> +             * conversion would do) is not exact.  So after having
> +             * checked that y is an integer in the int64_t range
> +             * (i.e. that it is within bounds and its fractional part
> +             * is zero), compare both as integers. */
> +            return num_y->u.dbl >= -0x1p63 && num_y->u.dbl < 0x1p63 &&
> +                modf(num_y->u.dbl, &integral_part) == 0.0 &&

'man modf': given modf(x, &iptr), if x is a NaN, a Nan is returned
(good, NaN, is never equal to any integer value). But if x is positive
infinity, +0 is returned...

> +                num_x->u.i64 == (int64_t)num_y->u.dbl;

...and *iptr is set to positive infinity.  You are now converting
infinity to int64_t (whether via num_y->u.dbl or via &integral_part),
which falls in the unspecified portion of C99 (your quotes from 6.3.1.4
mentioned converting a finite value of real to integer, and say nothing
about converting NaN or infinity to integer).

Adding an 'isfinite(num_y->u.dbl) &&' to the expression would cover your
bases (or even 'isfinite(integral_part)', if we are worried about a
static checker complaining that we assign but never read integral_part).

> +        }
> +        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:
> +            /* Comparing x to y in double (which the implicit
> +             * conversion would do) is not exact.  So after having
> +             * checked that y is an integer in the uint64_t range
> +             * (i.e. that it is within bounds and its fractional part
> +             * is zero), compare both as integers. */
> +            return num_y->u.dbl >= 0 && num_y->u.dbl < 0x1p64 &&
> +                modf(num_y->u.dbl, &integral_part) == 0.0 &&
> +                num_x->u.u64 == (uint64_t)num_y->u.dbl;

And again.

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

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


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

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

[Eric Blake]

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

On 07/05/2017 02:04 PM, 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
> 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 | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 

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 v4 5/5] tests: Add check-qobject for equality tests

[Eric Blake]

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

On 07/05/2017 02:04 PM, Max Reitz wrote:
> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
> 
> Its only purpose for now is to test the qobject_is_equal() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/Makefile.include |   4 +-
>  qobject/qnum.c         |  16 +-
>  tests/check-qobject.c  | 404 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 417 insertions(+), 7 deletions(-)
>  create mode 100644 tests/check-qobject.c
> 

> +++ b/qobject/qnum.c
> @@ -217,12 +217,16 @@ QNum *qobject_to_qnum(const QObject *obj)
>  /**
>   * qnum_is_equal(): Test whether the two QNums are equal
>   *
> - * Negative integers are never considered equal to unsigned integers.
> - * Doubles are only considered equal to integers if their fractional
> - * part is zero and their integral part is exactly equal to the
> - * integer.  Because doubles have limited precision, there are
> - * therefore integers which do not have an equal double (e.g.
> - * INT64_MAX).
> + * This comparison is done independently of the internal
> + * representation.  Any two numbers are considered equal if they are
> + * mathmatically equal, that means:

s/mathmatically/mathematically/

> + * - Negative integers are never considered equal to unsigned
> + *   integers.
> + * - Floating point values are only considered equal to integers if
> + *   their fractional part is zero and their integral part is exactly
> + *   equal to the integer.  Because doubles have limited precision,
> + *   there are therefore integers which do not have an equal floating
> + *   point value (e.g. INT64_MAX).
>   */

> +static void qobject_is_equal_num_test(void)
> +{
> +    QNum *u0, *i0, *d0, *d0p25, *dnan, *um42, *im42, *dm42;

Given my comments on 2/5, do you want a dinf?

> +    QNum *umax, *imax, *umax_exact, *umax_exact_p1;
> +    QNum *dumax, *dimax, *dumax_exact, *dumax_exact_p1;
> +    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);

Are there compilers that complain if we open-code division by zero
instead of using NAN from <math.h> (similarly, if you test infinity, I'd
use the INFINITY macro instead of an open-coded computation)

> +    um42 = qnum_from_uint((uint64_t)-42);
> +    im42 = qnum_from_int(-42);
> +    dm42 = qnum_from_int(-42.0);
> +
> +    /* 2^64 - 1: Not exactly representable as a double (needs 64 bits
> +     * of precision, but double only has 53).  The double equivalent
> +     * may be either 2^64 or 2^64 - 2^11. */
> +    umax = qnum_from_uint(UINT64_MAX);
> +
> +    /* 2^63 - 1: Not exactly representable as a double (needs 63 bits
> +     * of precision, but double only has 53).  The double equivalent
> +     * may be either 2^63 or 2^63 - 2^10. */
> +    imax = qnum_from_int(INT64_MAX);
> +    /* 2^64 - 2^11: Exactly representable as a double (the least
> +     * significant 11 bits are set to 0, so we only need the 53 bits
> +     * of precision double offers).  This is the maximum value which
> +     * is exactly representable both as a uint64_t and a double. */
> +    umax_exact = qnum_from_uint(UINT64_MAX - 0x7ff);
> +
> +    /* 2^64 - 2^11 + 1: Not exactly representable as a double (needs
> +     * 64 bits again), but whereas (double)UINT64_MAX may be rounded
> +     * up to 2^64, this will most likely be rounded down to
> +     * 2^64 - 2^11. */
> +    umax_exact_p1 = qnum_from_uint(UINT64_MAX - 0x7ff + 1);

Nice.

> +
> +    dumax = qnum_from_double((double)qnum_get_uint(umax));
> +    dimax = qnum_from_double((double)qnum_get_int(imax));
> +    dumax_exact = qnum_from_double((double)qnum_get_uint(umax_exact));
> +    dumax_exact_p1 = qnum_from_double((double)qnum_get_uint(umax_exact_p1));

Compiler-dependent what values (some) of these doubles hold.

> +
> +    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 -- note however
> +     * that NaN cannot occur in a JSON object anyway. */
> +    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);

If you test infinity, that also cannot occur in JSON objects.

> +
> +    /* No unsigned overflow */
> +    test_equality(false, um42, im42);
> +    test_equality(false, um42, dm42);
> +    test_equality(true, im42, dm42);
> +
> +
> +    /*
> +     * Floating point values must match integers exactly to be
> +     * considered equal; it does not suffice that converting the
> +     * integer to a double yields the same value.
> +     * Each of the following four tests follows the same pattern:
> +     * 1. Check that both QNum objects compare unequal because they
> +     *    are (mathematically).  The third test is an exception,
> +     *    because here they are indeed equal.
> +     * 2. Check that when converting the integer QNum to a double,
> +     *    that value is equal to the double QNum.  We can thus see
> +     *    that the QNum comparison does not simply convert the
> +     *    integer to a floating point value (in a potentially lossy
> +     *    operation).
> +     * 3. Sanity checks: Check that the double QNum has the expected
> +     *    value (which may be one of two in case it was rounded; the
> +     *    exact result is then implementation-defined).
> +     *    If there are multiple valid values, check that they are
> +     *    distinct values when represented as double (just proving
> +     *    that our assumptions about the precision of doubles are
> +     *    correct).
> +     *
> +     * The first two tests are interesting because they may involve a
> +     * double value which is out of the uint64_t or int64_t range,
> +     * respectively (if it is rounded to 2^64 or 2^63 during
> +     * conversion).
> +     *
> +     * Since both are intended to involve rounding the value up during
> +     * conversion, we also have the fourth test which is indended to

s/indended/intended/

> +     * test behavior if the value was rounded down. This is the fourth
> +     * test.
> +     *
> +     * The third test simply proves that the value used in the fourth
> +     * test is indeed just one above a number that can be exactly
> +     * represented in a double.
> +     */
> +
> +    test_equality(false, umax, dumax);
> +    g_assert(qnum_get_double(umax) == qnum_get_double(dumax));
> +    g_assert(qnum_get_double(dumax) == 0x1p64 ||
> +             qnum_get_double(dumax) == 0x1p64 - 0x1p11);
> +    g_assert(0x1p64 != 0x1p64 - 0x1p11);
> +
> +    test_equality(false, imax, dimax);
> +    g_assert(qnum_get_double(imax) == qnum_get_double(dimax));
> +    g_assert(qnum_get_double(dimax) == 0x1p63 ||
> +             qnum_get_double(dimax) == 0x1p63 - 0x1p10);
> +    g_assert(0x1p63 != 0x1p63 - 0x1p10);
> +
> +    test_equality(true, umax_exact, dumax_exact);
> +    g_assert(qnum_get_double(umax_exact) == qnum_get_double(dumax_exact));
> +    g_assert(qnum_get_double(dumax_exact) == 0x1p64 - 0x1p11);
> +
> +    test_equality(false, umax_exact_p1, dumax_exact_p1);
> +    g_assert(qnum_get_double(umax_exact_p1) == qnum_get_double(dumax_exact_p1));
> +    g_assert(qnum_get_double(dumax_exact_p1) == 0x1p64 ||
> +             qnum_get_double(dumax_exact_p1) == 0x1p64 - 0x1p11);
> +    g_assert(0x1p64 != 0x1p64 - 0x1p11);

Okay, and you catered to the indeterminate nature of the compiler
rounding pointed out earlier in the creation of the various doubles.

So all-in-all, you may want to add tests for infinity (given the
undefined nature of casting infinity to integer and any impact to commit
2/5), but what you have looks good:
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 v4 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>
> ---
> Markus also proposed just reporting two values as unequal if they have a
> different internal representation (i.e. a different QNum kind).
>
> I don't like this very much, because I feel like QInt and QFloat have
> been unified for a reason: Outside of these classes, nobody should care
> about the exact internal representation.  In JSON, there is no
> difference anyway.  We probably want to use integers as long as we can
> and doubles whenever we cannot.

You're right in that JSON has no notion of integer and floating-point,
only "number".  RFC 4627 is famously useless[1] on what exactly a number
ought to be, and its successor RFC 7159 could then (due to wildly
varying existing practice) merely state that a number is what the
implementation makes it to be, and advises "good interoperability can be
achieved" by making it double".  Pffft.

For us, being able to represent 64 bit integers is more important than
interoperating with crappy JSON implementations, so we made it the union
of int64_t, uint64_t and double[2].

You make a fair point when you say that nothing outside QNum should care
about the exact internal representation.  Trouble is that unless I'm
mistaken, your idea of "care" doesn't match the existing code's idea.

Let i42 = qnum_from_int(42)
    u42 = qnum_from_uint(42)
    d42 = qnum_from_double(42)

Then

    qnum_is_equal(i42, u42) yields true, I think.
    qnum_is_equal(i42, d42) yields true, I think.
    qnum_get_int(i42) yields 42.
    qnum_get_int(u42) yields 42.
    qnum_get_int(d42) fails its assertion.

Failing an assertion qualifies as "care", doesn't it?

> In any case, I feel like the class should hide the different internal
> representations from the user.  This necessitates being able to compare
> floating point values against integers.  Since apparently the main use
> of QObject is to parse and emit JSON (and represent such objects
> internally), we also have to agree that JSON doesn't make a difference:
> 42 is just the same as 42.0.

The JSON RFC is mum on that.

In *our* implementation of JSON, 42 and 42.0 have always been very much
*not* the same.  Proof:

    -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
    <- {"return": {}}
    -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
    <- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'value', expected: integer"}}

This is because migrate_set_speed argument value is 'int', and 42.0 is
not a valid 'int' value.

Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
value is 'number':

    -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
    <- {"return": {}}
    -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
    <- {"return": {}}

Don't blame me for the parts of QMP I inherited :)

> Finally, I think it's rather pointless not to consider 42u and 42 the
> same value.  But since unsigned/signed are two different kinds of QNums
> already, we cannot consider them equal without considering 42.0 equal,
> too.

Non sequitur.

> Because of this, I have decided to continue to compare QNum values even
> if they are of a different kind.

I think comparing signed and unsigned integer QNums is fair and
consistent with how the rest of our code works.

Comparing integer and floating QNums isn't.  It's also a can of worms.
Are you sure we *need* to open that can *now*?

Are you sure a simple, stupid eql-like comparison won't do *for now*?
YAGNI!


[1] Standard reply to criticism of JSON: could be worse, could be XML.

[2] Union of int64_t and double until recently, plus bugs that could be
abused to "tunnel" uint64_t values.  Some of the bugs have to remain for
backward compatibility.

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

[Max Reitz]

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

On 2017-07-05 21:49, Eric Blake wrote:
> On 07/05/2017 02:04 PM, Max Reitz wrote:
>> This generic function (along with its implementations for different
>> types) determines whether two QObjects are equal.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Markus also proposed just reporting two values as unequal if they have a
>> different internal representation (i.e. a different QNum kind).
>>
>> I don't like this very much, because I feel like QInt and QFloat have
>> been unified for a reason: Outside of these classes, nobody should care
>> about the exact internal representation.  In JSON, there is no
>> difference anyway.  We probably want to use integers as long as we can
>> and doubles whenever we cannot.
>>
>> In any case, I feel like the class should hide the different internal
>> representations from the user.  This necessitates being able to compare
>> floating point values against integers.  Since apparently the main use
>> of QObject is to parse and emit JSON (and represent such objects
>> internally), we also have to agree that JSON doesn't make a difference:
>> 42 is just the same as 42.0.
>>
>> Finally, I think it's rather pointless not to consider 42u and 42 the
>> same value.  But since unsigned/signed are two different kinds of QNums
>> already, we cannot consider them equal without considering 42.0 equal,
>> too.
>>
>> Because of this, I have decided to continue to compare QNum values even
>> if they are of a different kind.
> 
> This explanation may deserve to be in the commit log proper.
> 
>>  /**
>> + * qnum_is_equal(): Test whether the two QNums are equal
>> + *
>> + * Negative integers are never considered equal to unsigned integers.
>> + * Doubles are only considered equal to integers if their fractional
>> + * part is zero and their integral part is exactly equal to the
>> + * integer.  Because doubles have limited precision, there are
>> + * therefore integers which do not have an equal double (e.g.
>> + * INT64_MAX).
>> + */
>> +bool qnum_is_equal(const QObject *x, const QObject *y)
>> +{
>> +    QNum *num_x = qobject_to_qnum(x);
>> +    QNum *num_y = qobject_to_qnum(y);
>> +    double integral_part; /* Needed for the modf() calls below */
>> +
>> +    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:
>> +            /* Comparing x to y in double (which the implicit
>> +             * conversion would do) is not exact.  So after having
>> +             * checked that y is an integer in the int64_t range
>> +             * (i.e. that it is within bounds and its fractional part
>> +             * is zero), compare both as integers. */
>> +            return num_y->u.dbl >= -0x1p63 && num_y->u.dbl < 0x1p63 &&
>> +                modf(num_y->u.dbl, &integral_part) == 0.0 &&
> 
> 'man modf': given modf(x, &iptr), if x is a NaN, a Nan is returned
> (good, NaN, is never equal to any integer value). But if x is positive
> infinity, +0 is returned...
> 
>> +                num_x->u.i64 == (int64_t)num_y->u.dbl;
> 
> ...and *iptr is set to positive infinity.  You are now converting
> infinity to int64_t (whether via num_y->u.dbl or via &integral_part),
> which falls in the unspecified portion of C99 (your quotes from 6.3.1.4
> mentioned converting a finite value of real to integer, and say nothing
> about converting NaN or infinity to integer).
> 
> Adding an 'isfinite(num_y->u.dbl) &&' to the expression would cover your
> bases (or even 'isfinite(integral_part)', if we are worried about a
> static checker complaining that we assign but never read integral_part).

Infinity is covered by the range check, though.

Max

> 
>> +        }
>> +        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:
>> +            /* Comparing x to y in double (which the implicit
>> +             * conversion would do) is not exact.  So after having
>> +             * checked that y is an integer in the uint64_t range
>> +             * (i.e. that it is within bounds and its fractional part
>> +             * is zero), compare both as integers. */
>> +            return num_y->u.dbl >= 0 && num_y->u.dbl < 0x1p64 &&
>> +                modf(num_y->u.dbl, &integral_part) == 0.0 &&
>> +                num_x->u.u64 == (uint64_t)num_y->u.dbl;
> 
> And again.
> 
> With that addition,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 



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

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

[Max Reitz]

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

On 2017-07-05 22:05, Eric Blake wrote:
> On 07/05/2017 02:04 PM, Max Reitz wrote:
>> Add a new test file (check-qobject.c) for unit tests that concern
>> QObjects as a whole.
>>
>> Its only purpose for now is to test the qobject_is_equal() function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/Makefile.include |   4 +-
>>  qobject/qnum.c         |  16 +-
>>  tests/check-qobject.c  | 404 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 417 insertions(+), 7 deletions(-)
>>  create mode 100644 tests/check-qobject.c
>>
> 
>> +++ b/qobject/qnum.c
>> @@ -217,12 +217,16 @@ QNum *qobject_to_qnum(const QObject *obj)
>>  /**
>>   * qnum_is_equal(): Test whether the two QNums are equal
>>   *
>> - * Negative integers are never considered equal to unsigned integers.
>> - * Doubles are only considered equal to integers if their fractional
>> - * part is zero and their integral part is exactly equal to the
>> - * integer.  Because doubles have limited precision, there are
>> - * therefore integers which do not have an equal double (e.g.
>> - * INT64_MAX).
>> + * This comparison is done independently of the internal
>> + * representation.  Any two numbers are considered equal if they are
>> + * mathmatically equal, that means:
> 
> s/mathmatically/mathematically/
> 
>> + * - Negative integers are never considered equal to unsigned
>> + *   integers.
>> + * - Floating point values are only considered equal to integers if
>> + *   their fractional part is zero and their integral part is exactly
>> + *   equal to the integer.  Because doubles have limited precision,
>> + *   there are therefore integers which do not have an equal floating
>> + *   point value (e.g. INT64_MAX).
>>   */
> 
>> +static void qobject_is_equal_num_test(void)
>> +{
>> +    QNum *u0, *i0, *d0, *d0p25, *dnan, *um42, *im42, *dm42;
> 
> Given my comments on 2/5, do you want a dinf?

If you give me an idea on what to do with them other to compare that one
infinite float equals another, sure.  I wouldn't know how which integers
to compare them against, though.

> 
>> +    QNum *umax, *imax, *umax_exact, *umax_exact_p1;
>> +    QNum *dumax, *dimax, *dumax_exact, *dumax_exact_p1;
>> +    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);
> 
> Are there compilers that complain if we open-code division by zero
> instead of using NAN from <math.h> (similarly, if you test infinity, I'd
> use the INFINITY macro instead of an open-coded computation)

Hm, true, it may trap, right? Well, why not use NAN then, sure.

>> +    um42 = qnum_from_uint((uint64_t)-42);
>> +    im42 = qnum_from_int(-42);
>> +    dm42 = qnum_from_int(-42.0);
>> +
>> +    /* 2^64 - 1: Not exactly representable as a double (needs 64 bits
>> +     * of precision, but double only has 53).  The double equivalent
>> +     * may be either 2^64 or 2^64 - 2^11. */
>> +    umax = qnum_from_uint(UINT64_MAX);
>> +
>> +    /* 2^63 - 1: Not exactly representable as a double (needs 63 bits
>> +     * of precision, but double only has 53).  The double equivalent
>> +     * may be either 2^63 or 2^63 - 2^10. */
>> +    imax = qnum_from_int(INT64_MAX);
>> +    /* 2^64 - 2^11: Exactly representable as a double (the least
>> +     * significant 11 bits are set to 0, so we only need the 53 bits
>> +     * of precision double offers).  This is the maximum value which
>> +     * is exactly representable both as a uint64_t and a double. */
>> +    umax_exact = qnum_from_uint(UINT64_MAX - 0x7ff);
>> +
>> +    /* 2^64 - 2^11 + 1: Not exactly representable as a double (needs
>> +     * 64 bits again), but whereas (double)UINT64_MAX may be rounded
>> +     * up to 2^64, this will most likely be rounded down to
>> +     * 2^64 - 2^11. */
>> +    umax_exact_p1 = qnum_from_uint(UINT64_MAX - 0x7ff + 1);
> 
> Nice.
> 
>> +
>> +    dumax = qnum_from_double((double)qnum_get_uint(umax));
>> +    dimax = qnum_from_double((double)qnum_get_int(imax));
>> +    dumax_exact = qnum_from_double((double)qnum_get_uint(umax_exact));
>> +    dumax_exact_p1 = qnum_from_double((double)qnum_get_uint(umax_exact_p1));
> 
> Compiler-dependent what values (some) of these doubles hold.

Yep.

>> +
>> +    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 -- note however
>> +     * that NaN cannot occur in a JSON object anyway. */
>> +    g_assert(qobject_is_equal(QOBJECT(dnan), QOBJECT(dnan)) == false);
> 
> If you test infinity, that also cannot occur in JSON objects.
> 
>> +
>> +    /* No unsigned overflow */
>> +    test_equality(false, um42, im42);
>> +    test_equality(false, um42, dm42);
>> +    test_equality(true, im42, dm42);
>> +
>> +
>> +    /*
>> +     * Floating point values must match integers exactly to be
>> +     * considered equal; it does not suffice that converting the
>> +     * integer to a double yields the same value.
>> +     * Each of the following four tests follows the same pattern:
>> +     * 1. Check that both QNum objects compare unequal because they
>> +     *    are (mathematically).  The third test is an exception,
>> +     *    because here they are indeed equal.
>> +     * 2. Check that when converting the integer QNum to a double,
>> +     *    that value is equal to the double QNum.  We can thus see
>> +     *    that the QNum comparison does not simply convert the
>> +     *    integer to a floating point value (in a potentially lossy
>> +     *    operation).
>> +     * 3. Sanity checks: Check that the double QNum has the expected
>> +     *    value (which may be one of two in case it was rounded; the
>> +     *    exact result is then implementation-defined).
>> +     *    If there are multiple valid values, check that they are
>> +     *    distinct values when represented as double (just proving
>> +     *    that our assumptions about the precision of doubles are
>> +     *    correct).
>> +     *
>> +     * The first two tests are interesting because they may involve a
>> +     * double value which is out of the uint64_t or int64_t range,
>> +     * respectively (if it is rounded to 2^64 or 2^63 during
>> +     * conversion).
>> +     *
>> +     * Since both are intended to involve rounding the value up during
>> +     * conversion, we also have the fourth test which is indended to
> 
> s/indended/intended/
> 
>> +     * test behavior if the value was rounded down. This is the fourth
>> +     * test.
>> +     *
>> +     * The third test simply proves that the value used in the fourth
>> +     * test is indeed just one above a number that can be exactly
>> +     * represented in a double.
>> +     */
>> +
>> +    test_equality(false, umax, dumax);
>> +    g_assert(qnum_get_double(umax) == qnum_get_double(dumax));
>> +    g_assert(qnum_get_double(dumax) == 0x1p64 ||
>> +             qnum_get_double(dumax) == 0x1p64 - 0x1p11);
>> +    g_assert(0x1p64 != 0x1p64 - 0x1p11);
>> +
>> +    test_equality(false, imax, dimax);
>> +    g_assert(qnum_get_double(imax) == qnum_get_double(dimax));
>> +    g_assert(qnum_get_double(dimax) == 0x1p63 ||
>> +             qnum_get_double(dimax) == 0x1p63 - 0x1p10);
>> +    g_assert(0x1p63 != 0x1p63 - 0x1p10);
>> +
>> +    test_equality(true, umax_exact, dumax_exact);
>> +    g_assert(qnum_get_double(umax_exact) == qnum_get_double(dumax_exact));
>> +    g_assert(qnum_get_double(dumax_exact) == 0x1p64 - 0x1p11);
>> +
>> +    test_equality(false, umax_exact_p1, dumax_exact_p1);
>> +    g_assert(qnum_get_double(umax_exact_p1) == qnum_get_double(dumax_exact_p1));
>> +    g_assert(qnum_get_double(dumax_exact_p1) == 0x1p64 ||
>> +             qnum_get_double(dumax_exact_p1) == 0x1p64 - 0x1p11);
>> +    g_assert(0x1p64 != 0x1p64 - 0x1p11);
> 
> Okay, and you catered to the indeterminate nature of the compiler
> rounding pointed out earlier in the creation of the various doubles.
> 
> So all-in-all, you may want to add tests for infinity (given the
> undefined nature of casting infinity to integer and any impact to commit
> 2/5), but what you have looks good:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Adding infinity sounds good, but I wouldn't know what tests to do with
it...   So unless I come up with something, I'll at least make the test
use NAN and fix the spelling issues.

Thanks!

Max


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

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

[Max Reitz]

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

On 2017-07-06 16:30, 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>
>> ---
>> Markus also proposed just reporting two values as unequal if they have a
>> different internal representation (i.e. a different QNum kind).
>>
>> I don't like this very much, because I feel like QInt and QFloat have
>> been unified for a reason: Outside of these classes, nobody should care
>> about the exact internal representation.  In JSON, there is no
>> difference anyway.  We probably want to use integers as long as we can
>> and doubles whenever we cannot.
> 
> You're right in that JSON has no notion of integer and floating-point,
> only "number".  RFC 4627 is famously useless[1] on what exactly a number
> ought to be, and its successor RFC 7159 could then (due to wildly
> varying existing practice) merely state that a number is what the
> implementation makes it to be, and advises "good interoperability can be
> achieved" by making it double".  Pffft.
> 
> For us, being able to represent 64 bit integers is more important than
> interoperating with crappy JSON implementations, so we made it the union
> of int64_t, uint64_t and double[2].
> 
> You make a fair point when you say that nothing outside QNum should care
> about the exact internal representation.  Trouble is that unless I'm
> mistaken, your idea of "care" doesn't match the existing code's idea.

I disagree that it doesn't match the existing code's idea.  I think the
existing code doesn't match its idea, but mine does.

> Let i42 = qnum_from_int(42)
>     u42 = qnum_from_uint(42)
>     d42 = qnum_from_double(42)
> 
> Then
> 
>     qnum_is_equal(i42, u42) yields true, I think.
>     qnum_is_equal(i42, d42) yields true, I think.
>     qnum_get_int(i42) yields 42.
>     qnum_get_int(u42) yields 42.
>     qnum_get_int(d42) fails its assertion.
> 
> Failing an assertion qualifies as "care", doesn't it?

It doesn't convert the value?  That's definitely not what I would have
thought and it doesn't make a lot of sense to me.  I call that a bug. :-)

From the other side we see that qnum_get_double() on all of this would
yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
Because there are doubles you cannot reasonably convert to integers, I
presume, whereas the other way around the worst that can happen is that
you lose some precision.

But that has no implication on qnum_is_equal().  If the double cannot be
converted to an integer because it is out of bounds, the values just are
not equal.  Simple.

So since qnum_get_double() does a conversion, I very much think that the
reason qnum_get_int() doesn't is mostly "because sometimes it's not
reasonably possible" and very much not because it is not intended to.

>> In any case, I feel like the class should hide the different internal
>> representations from the user.  This necessitates being able to compare
>> floating point values against integers.  Since apparently the main use
>> of QObject is to parse and emit JSON (and represent such objects
>> internally), we also have to agree that JSON doesn't make a difference:
>> 42 is just the same as 42.0.
> 
> The JSON RFC is mum on that.
> 
> In *our* implementation of JSON, 42 and 42.0 have always been very much
> *not* the same.  Proof:
> 
>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
>     <- {"return": {}}
>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
>     <- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'value', expected: integer"}}
> 
> This is because migrate_set_speed argument value is 'int', and 42.0 is
> not a valid 'int' value.

Well, that's a bug, too.  It's nice that we accept things that aren't
quite valid JSON (I'm looking at you, single quote), but we should
accept things that are valid JSON.

> Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
> value is 'number':
> 
>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
>     <- {"return": {}}
>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
>     <- {"return": {}}
> 
> Don't blame me for the parts of QMP I inherited :)

I sure don't.  But I am willing to start a discussion by calling that a
bug. ;-)

QNum has only been introduced recently.  Before, we had a hard split of
QInt and QFloat.  So I'm not surprised that we haven't fixed everything yet.

OTOH the introduction of QNum to me signals that we do want to fix this
eventually.

>> Finally, I think it's rather pointless not to consider 42u and 42 the
>> same value.  But since unsigned/signed are two different kinds of QNums
>> already, we cannot consider them equal without considering 42.0 equal,
>> too.
> 
> Non sequitur.
> 
>> Because of this, I have decided to continue to compare QNum values even
>> if they are of a different kind.
> 
> I think comparing signed and unsigned integer QNums is fair and
> consistent with how the rest of our code works.

I don't see how. doubles can represent different numbers than integers
can. Signed integers can represent different numbers than unsigned can.

Sure, signed/unsigned makes less of a difference than having an exponent
does.  But I don't agree we should make a difference when the only
reason not to seems to be "qemu currently likes to make a difference in
its interface, for historical reasons mainly" and "Do you really want to
write this equality function?  It seems hard to get right".

For the record, I could have lived with the old separation into QInt and
QFloat.  But now we do have a common QNum and I think the idea behind is
is to have a uniform opaque interface.

> Comparing integer and floating QNums isn't.  It's also a can of worms.
> Are you sure we *need* to open that can *now*?

Sure?  No.  Do I want to?  I guess so.

> Are you sure a simple, stupid eql-like comparison won't do *for now*?
> YAGNI!

But I want it.  I think the current behavior your demonstrated above is
a bug and I don't really want to continue to follow it.

All you have really convinced me to do is to add another patch which
smacks a warning on qnum_get_int(), and maybe even a TODO that it should
convert doubles to integers *if possible*.

(And the "if possible" just means that you cannot convert values which
are out of bounds or NaN.  Fractional parts may not even matter much --
I mean, we do happily convert integers to doubles and rounding that way
is implementation-defined.)

Max

> [1] Standard reply to criticism of JSON: could be worse, could be XML.
> 
> [2] Union of int64_t and double until recently, plus bugs that could be
> abused to "tunnel" uint64_t values.  Some of the bugs have to remain for
> backward compatibility.


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

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-07-06 16:30, 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>
>>> ---
>>> Markus also proposed just reporting two values as unequal if they have a
>>> different internal representation (i.e. a different QNum kind).
>>>
>>> I don't like this very much, because I feel like QInt and QFloat have
>>> been unified for a reason: Outside of these classes, nobody should care
>>> about the exact internal representation.  In JSON, there is no
>>> difference anyway.  We probably want to use integers as long as we can
>>> and doubles whenever we cannot.
>> 
>> You're right in that JSON has no notion of integer and floating-point,
>> only "number".  RFC 4627 is famously useless[1] on what exactly a number
>> ought to be, and its successor RFC 7159 could then (due to wildly
>> varying existing practice) merely state that a number is what the
>> implementation makes it to be, and advises "good interoperability can be
>> achieved" by making it double".  Pffft.
>> 
>> For us, being able to represent 64 bit integers is more important than
>> interoperating with crappy JSON implementations, so we made it the union
>> of int64_t, uint64_t and double[2].
>> 
>> You make a fair point when you say that nothing outside QNum should care
>> about the exact internal representation.  Trouble is that unless I'm
>> mistaken, your idea of "care" doesn't match the existing code's idea.
>
> I disagree that it doesn't match the existing code's idea.  I think the
> existing code doesn't match its idea, but mine does.
>
>> Let i42 = qnum_from_int(42)
>>     u42 = qnum_from_uint(42)
>>     d42 = qnum_from_double(42)
>> 
>> Then
>> 
>>     qnum_is_equal(i42, u42) yields true, I think.
>>     qnum_is_equal(i42, d42) yields true, I think.
>>     qnum_get_int(i42) yields 42.
>>     qnum_get_int(u42) yields 42.
>>     qnum_get_int(d42) fails its assertion.
>> 
>> Failing an assertion qualifies as "care", doesn't it?
>
> It doesn't convert the value?  That's definitely not what I would have
> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)

It's the existing code's idea, going back all the way to the dawn of
QMP: integers and floating point numbers are distinct.

Yes, they aren't distinct in the JSON grammar.  So sue the designers of
QMP.

Yes, they are less distinct in QMP than say integers and strings,
because there's an automatic conversion from integer to floating point.
Doesn't make them non-distinct; there is no conversion from floating
point to integer.

Yes, we recently changed the code to use the same C type for both.  That
was done to keep the code simple, not to change the semantics of QMP.

> From the other side we see that qnum_get_double() on all of this would
> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
> Because there are doubles you cannot reasonably convert to integers, I
> presume, whereas the other way around the worst that can happen is that
> you lose some precision.
>
> But that has no implication on qnum_is_equal().  If the double cannot be
> converted to an integer because it is out of bounds, the values just are
> not equal.  Simple.
>
> So since qnum_get_double() does a conversion, I very much think that the
> reason qnum_get_int() doesn't is mostly "because sometimes it's not
> reasonably possible" and very much not because it is not intended to.

It doesn't because the whole shebang is for QMP, and QMP does not ever
treat floating point numbers (numbers with decimal point or exponent) as
integers.

Yes, there are users other than QMP.  They adopted it because it was
convenient.  They thus adopted its oddities due to QMP's requirements,
too.

>>> In any case, I feel like the class should hide the different internal
>>> representations from the user.  This necessitates being able to compare
>>> floating point values against integers.  Since apparently the main use
>>> of QObject is to parse and emit JSON (and represent such objects
>>> internally), we also have to agree that JSON doesn't make a difference:
>>> 42 is just the same as 42.0.
>> 
>> The JSON RFC is mum on that.
>> 
>> In *our* implementation of JSON, 42 and 42.0 have always been very much
>> *not* the same.  Proof:
>> 
>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
>>     <- {"return": {}}
>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
>>     <- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'value', expected: integer"}}
>> 
>> This is because migrate_set_speed argument value is 'int', and 42.0 is
>> not a valid 'int' value.
>
> Well, that's a bug, too.  It's nice that we accept things that aren't
> quite valid JSON (I'm looking at you, single quote), but we should
> accept things that are valid JSON.

The fact that an expression is valid JSON does not oblige the
application to accept it!

Of all the valid JSON strings, the parser accepts only the ones shorter
than MAX_TOKEN_SIZE.  Command block-job-set-speed then rejects all but
the ones that happen to be valid job IDs.

Similarly, of all the valid JSON numbers, the parser accepts only the
integers (no decimal point, no exponent) that fit into int64_t, uint64_t
or double, and the floating point numbers (decimal point or exponent)
that fit into double.  Command migrate_set_speed then rejects all but
the integers (again, no decimal point, no exponent) between 0 and
SIZE_MAX.

JSON defines *syntax*.  Once again, the JSON RFC is mum on whether 42
and 42.0 are identical or distinct.  That's *semantics*, and semantics
are up to the application.  Ours has always treated them as distinct.
It is how QMP works.  We can like it or we can hate it.  I certainly
find plenty of things to dislike there myself.  What we can't do is deny
that it's ABI.

We can of course make ABI more accepting.  However, messing with the QMP
ABI is *hairy*, and we should therefore mess with it only when we have a
damn good practical reason.  "It's not nice" ain't.

>> Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
>> value is 'number':
>> 
>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
>>     <- {"return": {}}
>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
>>     <- {"return": {}}
>> 
>> Don't blame me for the parts of QMP I inherited :)
>
> I sure don't.  But I am willing to start a discussion by calling that a
> bug. ;-)
>
> QNum has only been introduced recently.  Before, we had a hard split of
> QInt and QFloat.  So I'm not surprised that we haven't fixed everything yet.
>
> OTOH the introduction of QNum to me signals that we do want to fix this
> eventually.

QNum was introduced to get us unsigned numbers with the least possible
notational overhead.  It wasn't introduced to signal intent to redesign
QMP numbers.

>>> Finally, I think it's rather pointless not to consider 42u and 42 the
>>> same value.  But since unsigned/signed are two different kinds of QNums
>>> already, we cannot consider them equal without considering 42.0 equal,
>>> too.
>> 
>> Non sequitur.
>> 
>>> Because of this, I have decided to continue to compare QNum values even
>>> if they are of a different kind.
>> 
>> I think comparing signed and unsigned integer QNums is fair and
>> consistent with how the rest of our code works.
>
> I don't see how. doubles can represent different numbers than integers
> can. Signed integers can represent different numbers than unsigned can.

The only way to add unsigned integers without breaking QMP compatibility
is to make them interchangeable with signed integers.  That doesn't mean
you get to make floating-point numbers interchangeable with integers
now.

> Sure, signed/unsigned makes less of a difference than having an exponent
> does.  But I don't agree we should make a difference when the only
> reason not to seems to be "qemu currently likes to make a difference in
> its interface, for historical reasons mainly" and "Do you really want to
> write this equality function?  It seems hard to get right".

"Because this is an interesting puzzle I'd love to solve" is wholly
insufficient reason to mess with QMP ABI.  It's also an insufficient
reason to add "interesting" code for me to maintain.

> For the record, I could have lived with the old separation into QInt and
> QFloat.  But now we do have a common QNum and I think the idea behind is
> is to have a uniform opaque interface.

Nope, the idea is to get unsigned integers through QMP with the least
notational overhead.

>> Comparing integer and floating QNums isn't.  It's also a can of worms.
>> Are you sure we *need* to open that can *now*?
>
> Sure?  No.  Do I want to?  I guess so.
>
>> Are you sure a simple, stupid eql-like comparison won't do *for now*?
>> YAGNI!
>
> But I want it.  I think the current behavior your demonstrated above is
> a bug and I don't really want to continue to follow it.

Feel free to call the current behavior a bug.  But it's a design bug
then.  Fixing design bugs in ABIs is somewhere between hard and
impractical.  I do not think this one is worth your while or mine.

> All you have really convinced me to do is to add another patch which
> smacks a warning on qnum_get_int(), and maybe even a TODO that it should
> convert doubles to integers *if possible*.
>
> (And the "if possible" just means that you cannot convert values which
> are out of bounds or NaN.  Fractional parts may not even matter much --
> I mean, we do happily convert integers to doubles and rounding that way
> is implementation-defined.)

Always try the stupidest solution that could possibly work first.
Unless I misunderstand your use case, a simple & stupid
qobject_is_equal() would do.  So let's try that first.

Adding capability to compare signed and unsigned integers should still
be fairly simple.  I'd be willing to consider it.

>> [1] Standard reply to criticism of JSON: could be worse, could be XML.
>> 
>> [2] Union of int64_t and double until recently, plus bugs that could be
>> abused to "tunnel" uint64_t values.  Some of the bugs have to remain for
>> backward compatibility.

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

[Max Reitz]

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

First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
least not right now and in the foreseeable future).
You're the maintainer, so you decide, so I'll go along with it. :-)

Now, let's follow up with my therefore rather useless commentary:

(Feel free to disregard, because honestly, I can see how replying to
most of the points I'm asking isn't really worth the time...)

On 2017-07-10 11:17, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2017-07-06 16:30, 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>
>>>> ---
>>>> Markus also proposed just reporting two values as unequal if they have a
>>>> different internal representation (i.e. a different QNum kind).
>>>>
>>>> I don't like this very much, because I feel like QInt and QFloat have
>>>> been unified for a reason: Outside of these classes, nobody should care
>>>> about the exact internal representation.  In JSON, there is no
>>>> difference anyway.  We probably want to use integers as long as we can
>>>> and doubles whenever we cannot.
>>>
>>> You're right in that JSON has no notion of integer and floating-point,
>>> only "number".  RFC 4627 is famously useless[1] on what exactly a number
>>> ought to be, and its successor RFC 7159 could then (due to wildly
>>> varying existing practice) merely state that a number is what the
>>> implementation makes it to be, and advises "good interoperability can be
>>> achieved" by making it double".  Pffft.
>>>
>>> For us, being able to represent 64 bit integers is more important than
>>> interoperating with crappy JSON implementations, so we made it the union
>>> of int64_t, uint64_t and double[2].
>>>
>>> You make a fair point when you say that nothing outside QNum should care
>>> about the exact internal representation.  Trouble is that unless I'm
>>> mistaken, your idea of "care" doesn't match the existing code's idea.
>>
>> I disagree that it doesn't match the existing code's idea.  I think the
>> existing code doesn't match its idea, but mine does.
>>
>>> Let i42 = qnum_from_int(42)
>>>     u42 = qnum_from_uint(42)
>>>     d42 = qnum_from_double(42)
>>>
>>> Then
>>>
>>>     qnum_is_equal(i42, u42) yields true, I think.
>>>     qnum_is_equal(i42, d42) yields true, I think.
>>>     qnum_get_int(i42) yields 42.
>>>     qnum_get_int(u42) yields 42.
>>>     qnum_get_int(d42) fails its assertion.
>>>
>>> Failing an assertion qualifies as "care", doesn't it?
>>
>> It doesn't convert the value?  That's definitely not what I would have
>> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)
> 
> It's the existing code's idea, going back all the way to the dawn of
> QMP: integers and floating point numbers are distinct.
> 
> Yes, they aren't distinct in the JSON grammar.  So sue the designers of
> QMP.

Sounds like it was a reasonable idea at the time but could be done
better today.  But that's how it always is, right?

> Yes, they are less distinct in QMP than say integers and strings,
> because there's an automatic conversion from integer to floating point.
> Doesn't make them non-distinct; there is no conversion from floating
> point to integer.

I can very well see that as a technical reason, but OK.

> Yes, we recently changed the code to use the same C type for both.  That
> was done to keep the code simple, not to change the semantics of QMP.

Hm, OK.

>> From the other side we see that qnum_get_double() on all of this would
>> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
>> Because there are doubles you cannot reasonably convert to integers, I
>> presume, whereas the other way around the worst that can happen is that
>> you lose some precision.
>>
>> But that has no implication on qnum_is_equal().  If the double cannot be
>> converted to an integer because it is out of bounds, the values just are
>> not equal.  Simple.
>>
>> So since qnum_get_double() does a conversion, I very much think that the
>> reason qnum_get_int() doesn't is mostly "because sometimes it's not
>> reasonably possible" and very much not because it is not intended to.
> 
> It doesn't because the whole shebang is for QMP, and QMP does not ever
> treat floating point numbers (numbers with decimal point or exponent) as
> integers.

Well, to my defense, I couldn't see that from looking at the code.  From
that point of view, it just looks like qnum_get_int() is lacking.

> Yes, there are users other than QMP.  They adopted it because it was
> convenient.  They thus adopted its oddities due to QMP's requirements,
> too.

To me, that mostly sounds like an excuse that distinguishing between
integers and floats will not be wrong, but not like a reason it is right.

>>>> In any case, I feel like the class should hide the different internal
>>>> representations from the user.  This necessitates being able to compare
>>>> floating point values against integers.  Since apparently the main use
>>>> of QObject is to parse and emit JSON (and represent such objects
>>>> internally), we also have to agree that JSON doesn't make a difference:
>>>> 42 is just the same as 42.0.
>>>
>>> The JSON RFC is mum on that.
>>>
>>> In *our* implementation of JSON, 42 and 42.0 have always been very much
>>> *not* the same.  Proof:
>>>
>>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
>>>     <- {"return": {}}
>>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
>>>     <- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'value', expected: integer"}}
>>>
>>> This is because migrate_set_speed argument value is 'int', and 42.0 is
>>> not a valid 'int' value.
>>
>> Well, that's a bug, too.  It's nice that we accept things that aren't
>> quite valid JSON (I'm looking at you, single quote), but we should
>> accept things that are valid JSON.
> 
> The fact that an expression is valid JSON does not oblige the
> application to accept it!

Err, well...

> Of all the valid JSON strings, the parser accepts only the ones shorter
> than MAX_TOKEN_SIZE.  Command block-job-set-speed then rejects all but
> the ones that happen to be valid job IDs.

Yes...

> Similarly, of all the valid JSON numbers, the parser accepts only the
> integers (no decimal point, no exponent) that fit into int64_t, uint64_t
> or double, and the floating point numbers (decimal point or exponent)
> that fit into double.  Command migrate_set_speed then rejects all but
> the integers (again, no decimal point, no exponent) between 0 and
> SIZE_MAX.
> 
> JSON defines *syntax*.  Once again, the JSON RFC is mum on whether 42
> and 42.0 are identical or distinct.  That's *semantics*, and semantics
> are up to the application.  Ours has always treated them as distinct.
> It is how QMP works.  We can like it or we can hate it.  I certainly
> find plenty of things to dislike there myself.  What we can't do is deny
> that it's ABI.

OK, yes, but I think it's just weird and serves no purpose.

The thing is that numbers are a special case.  As far as I can see, all
other parts of JSON have a clear and unique representation (disregarding
whitespace).  There is only one true, one false, one null, one way to
write a string, etc..

But there are many ways to write 42.  You can write 42, you can write
42.0, you can write 4.2e1.

This is very much guesswork on my part, but from what I've gathered
about JSON, there is no difference between integers and floats.  There
are only numbers.  So whatever interprets a JSON value semantically will
just see something that is a number value and it should not be able to
tell whether that number had a decimal point or not (except for guessing
by looking whether there's a fractional part).

Therefore, if you reject a number based on the fact that it has a
decimal point in it, that to me seems like syntax, not semantics.

In any case, to me it's no different from discriminating between 42.0
and 4.2e1 (which even in C is exactly the same value).

> We can of course make ABI more accepting.  However, messing with the QMP
> ABI is *hairy*, and we should therefore mess with it only when we have a
> damn good practical reason.  "It's not nice" ain't.

That depends on who looks at it.  You don't think it's a good reason,
OK.  I think it is.

I hope you can excuse me for not having yet made my fair share of bad
experiences with trying to fix things and thus breaking them even
further.  I'm sure I'll get appropriately pessimistic over the coming
years. :-)

>>> Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
>>> value is 'number':
>>>
>>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
>>>     <- {"return": {}}
>>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
>>>     <- {"return": {}}
>>>
>>> Don't blame me for the parts of QMP I inherited :)
>>
>> I sure don't.  But I am willing to start a discussion by calling that a
>> bug. ;-)
>>
>> QNum has only been introduced recently.  Before, we had a hard split of
>> QInt and QFloat.  So I'm not surprised that we haven't fixed everything yet.
>>
>> OTOH the introduction of QNum to me signals that we do want to fix this
>> eventually.
> 
> QNum was introduced to get us unsigned numbers with the least possible
> notational overhead.  It wasn't introduced to signal intent to redesign
> QMP numbers.

Again, that is very much not obvious from looking at QNum.  Why does it
include floats then?  Because some basically integer values were
represented as floats because they were supposed to be unsigned and did
not fit into an int64_t?

I could understand that from a technical perspective, but it sounds more
like we should have expanded QInt then to cover both signed and unsigned
integers and then fixed places which tried to "abuse" QFloat for
unsigned integers.

>>>> Finally, I think it's rather pointless not to consider 42u and 42 the
>>>> same value.  But since unsigned/signed are two different kinds of QNums
>>>> already, we cannot consider them equal without considering 42.0 equal,
>>>> too.
>>>
>>> Non sequitur.
>>>
>>>> Because of this, I have decided to continue to compare QNum values even
>>>> if they are of a different kind.
>>>
>>> I think comparing signed and unsigned integer QNums is fair and
>>> consistent with how the rest of our code works.
>>
>> I don't see how. doubles can represent different numbers than integers
>> can. Signed integers can represent different numbers than unsigned can.
> 
> The only way to add unsigned integers without breaking QMP compatibility
> is to make them interchangeable with signed integers.  That doesn't mean
> you get to make floating-point numbers interchangeable with integers
> now.

Again, begs the question why QNum covers floating point numbers then and
why this very fact is not documented in qnum.c.

>> Sure, signed/unsigned makes less of a difference than having an exponent
>> does.  But I don't agree we should make a difference when the only
>> reason not to seems to be "qemu currently likes to make a difference in
>> its interface, for historical reasons mainly" and "Do you really want to
>> write this equality function?  It seems hard to get right".
> 
> "Because this is an interesting puzzle I'd love to solve" is wholly
> insufficient reason to mess with QMP ABI.

I don't see how I'm messing with the QMP ABI here, but with an
s/QMP ABI/this/, I see your point.

>                                            It's also an insufficient
> reason to add "interesting" code for me to maintain.

Now this is a point I can fully understand and agree on.

>> For the record, I could have lived with the old separation into QInt and
>> QFloat.  But now we do have a common QNum and I think the idea behind is
>> is to have a uniform opaque interface.
> 
> Nope, the idea is to get unsigned integers through QMP with the least
> notational overhead.

(Again, why include floats, then?)

>>> Comparing integer and floating QNums isn't.  It's also a can of worms.
>>> Are you sure we *need* to open that can *now*?
>>
>> Sure?  No.  Do I want to?  I guess so.
>>
>>> Are you sure a simple, stupid eql-like comparison won't do *for now*?
>>> YAGNI!
>>
>> But I want it.  I think the current behavior your demonstrated above is
>> a bug and I don't really want to continue to follow it.
> 
> Feel free to call the current behavior a bug.  But it's a design bug
> then.  Fixing design bugs in ABIs is somewhere between hard and
> impractical.  I do not think this one is worth your while or mine.

Technical question: How is this an ABI and not an API?  Making QNum
replace QInt and QFloat was messing with the ABI.  Now, making QNum
behave as both depending on what is asked for is just an API change,
isn't it?

Also, I still don't see how just converting every JSON number into a
QNum and then making QNum return a valid integer or float depending on
who's asking would be hard or impractical.
(But really, don't bother to reply.  I pretty much know I'm overlooking
a lot here and this is just my naive standpoint.  Again, though, maybe
there should be documentation in qnum.c about this.)

>> All you have really convinced me to do is to add another patch which
>> smacks a warning on qnum_get_int(), and maybe even a TODO that it should
>> convert doubles to integers *if possible*.
>>
>> (And the "if possible" just means that you cannot convert values which
>> are out of bounds or NaN.  Fractional parts may not even matter much --
>> I mean, we do happily convert integers to doubles and rounding that way
>> is implementation-defined.)
> 
> Always try the stupidest solution that could possibly work first.
> Unless I misunderstand your use case, a simple & stupid
> qobject_is_equal() would do.  So let's try that first.
Honestly, I pretty much hate it.  But I can't say I disagree with your
most important points (it'd be useless, it'd be overly complicated,
you'd have to maintain something you don't want), so yep, will do.

> Adding capability to compare signed and unsigned integers should still
> be fairly simple.  I'd be willing to consider it.

Thanks for bearing with me. :-)

Max


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

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
> least not right now and in the foreseeable future).
> You're the maintainer, so you decide, so I'll go along with it. :-)
>
> Now, let's follow up with my therefore rather useless commentary:
>
> (Feel free to disregard, because honestly, I can see how replying to
> most of the points I'm asking isn't really worth the time...)

When I use the authority entrusted to maintainers, I feel obliged to at
least explain my reasoning.  Besides, putting my reasoning in words
tends to lead me to new insights.

> On 2017-07-10 11:17, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 2017-07-06 16:30, 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>
>>>>> ---
>>>>> Markus also proposed just reporting two values as unequal if they have a
>>>>> different internal representation (i.e. a different QNum kind).
>>>>>
>>>>> I don't like this very much, because I feel like QInt and QFloat have
>>>>> been unified for a reason: Outside of these classes, nobody should care
>>>>> about the exact internal representation.  In JSON, there is no
>>>>> difference anyway.  We probably want to use integers as long as we can
>>>>> and doubles whenever we cannot.
>>>>
>>>> You're right in that JSON has no notion of integer and floating-point,
>>>> only "number".  RFC 4627 is famously useless[1] on what exactly a number
>>>> ought to be, and its successor RFC 7159 could then (due to wildly
>>>> varying existing practice) merely state that a number is what the
>>>> implementation makes it to be, and advises "good interoperability can be
>>>> achieved" by making it double".  Pffft.
>>>>
>>>> For us, being able to represent 64 bit integers is more important than
>>>> interoperating with crappy JSON implementations, so we made it the union
>>>> of int64_t, uint64_t and double[2].
>>>>
>>>> You make a fair point when you say that nothing outside QNum should care
>>>> about the exact internal representation.  Trouble is that unless I'm
>>>> mistaken, your idea of "care" doesn't match the existing code's idea.
>>>
>>> I disagree that it doesn't match the existing code's idea.  I think the
>>> existing code doesn't match its idea, but mine does.
>>>
>>>> Let i42 = qnum_from_int(42)
>>>>     u42 = qnum_from_uint(42)
>>>>     d42 = qnum_from_double(42)
>>>>
>>>> Then
>>>>
>>>>     qnum_is_equal(i42, u42) yields true, I think.
>>>>     qnum_is_equal(i42, d42) yields true, I think.
>>>>     qnum_get_int(i42) yields 42.
>>>>     qnum_get_int(u42) yields 42.
>>>>     qnum_get_int(d42) fails its assertion.
>>>>
>>>> Failing an assertion qualifies as "care", doesn't it?
>>>
>>> It doesn't convert the value?  That's definitely not what I would have
>>> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)
>> 
>> It's the existing code's idea, going back all the way to the dawn of
>> QMP: integers and floating point numbers are distinct.
>> 
>> Yes, they aren't distinct in the JSON grammar.  So sue the designers of
>> QMP.
>
> Sounds like it was a reasonable idea at the time but could be done
> better today.  But that's how it always is, right?

Finding fault with designs we've inherited turns out to be much easier
than coming up with designs that last.

>> Yes, they are less distinct in QMP than say integers and strings,
>> because there's an automatic conversion from integer to floating point.
>> Doesn't make them non-distinct; there is no conversion from floating
>> point to integer.
>
> I can very well see that as a technical reason, but OK.
>
>> Yes, we recently changed the code to use the same C type for both.  That
>> was done to keep the code simple, not to change the semantics of QMP.
>
> Hm, OK.
>
>>> From the other side we see that qnum_get_double() on all of this would
>>> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
>>> Because there are doubles you cannot reasonably convert to integers, I
>>> presume, whereas the other way around the worst that can happen is that
>>> you lose some precision.
>>>
>>> But that has no implication on qnum_is_equal().  If the double cannot be
>>> converted to an integer because it is out of bounds, the values just are
>>> not equal.  Simple.
>>>
>>> So since qnum_get_double() does a conversion, I very much think that the
>>> reason qnum_get_int() doesn't is mostly "because sometimes it's not
>>> reasonably possible" and very much not because it is not intended to.
>> 
>> It doesn't because the whole shebang is for QMP, and QMP does not ever
>> treat floating point numbers (numbers with decimal point or exponent) as
>> integers.
>
> Well, to my defense, I couldn't see that from looking at the code.  From
> that point of view, it just looks like qnum_get_int() is lacking.

Looking at qnum.c in isolation can certainly lead sensible people to
such ideas.

>> Yes, there are users other than QMP.  They adopted it because it was
>> convenient.  They thus adopted its oddities due to QMP's requirements,
>> too.
>
> To me, that mostly sounds like an excuse that distinguishing between
> integers and floats will not be wrong, but not like a reason it is right.

More on that below.

>>>>> In any case, I feel like the class should hide the different internal
>>>>> representations from the user.  This necessitates being able to compare
>>>>> floating point values against integers.  Since apparently the main use
>>>>> of QObject is to parse and emit JSON (and represent such objects
>>>>> internally), we also have to agree that JSON doesn't make a difference:
>>>>> 42 is just the same as 42.0.
>>>>
>>>> The JSON RFC is mum on that.
>>>>
>>>> In *our* implementation of JSON, 42 and 42.0 have always been very much
>>>> *not* the same.  Proof:
>>>>
>>>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
>>>>     <- {"return": {}}
>>>>     -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
>>>>     <- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'value', expected: integer"}}
>>>>
>>>> This is because migrate_set_speed argument value is 'int', and 42.0 is
>>>> not a valid 'int' value.
>>>
>>> Well, that's a bug, too.  It's nice that we accept things that aren't
>>> quite valid JSON (I'm looking at you, single quote), but we should
>>> accept things that are valid JSON.
>> 
>> The fact that an expression is valid JSON does not oblige the
>> application to accept it!
>
> Err, well...
>
>> Of all the valid JSON strings, the parser accepts only the ones shorter
>> than MAX_TOKEN_SIZE.  Command block-job-set-speed then rejects all but
>> the ones that happen to be valid job IDs.
>
> Yes...
>
>> Similarly, of all the valid JSON numbers, the parser accepts only the
>> integers (no decimal point, no exponent) that fit into int64_t, uint64_t
>> or double, and the floating point numbers (decimal point or exponent)
>> that fit into double.  Command migrate_set_speed then rejects all but
>> the integers (again, no decimal point, no exponent) between 0 and
>> SIZE_MAX.
>> 
>> JSON defines *syntax*.  Once again, the JSON RFC is mum on whether 42
>> and 42.0 are identical or distinct.  That's *semantics*, and semantics
>> are up to the application.  Ours has always treated them as distinct.
>> It is how QMP works.  We can like it or we can hate it.  I certainly
>> find plenty of things to dislike there myself.  What we can't do is deny
>> that it's ABI.
>
> OK, yes, but I think it's just weird and serves no purpose.
>
> The thing is that numbers are a special case.  As far as I can see, all
> other parts of JSON have a clear and unique representation (disregarding
> whitespace).  There is only one true, one false, one null, one way to
> write a string, etc..
>
> But there are many ways to write 42.  You can write 42, you can write
> 42.0, you can write 4.2e1.
>
> This is very much guesswork on my part, but from what I've gathered
> about JSON, there is no difference between integers and floats.  There
> are only numbers.

I'm afraid you've gathered wrong.

RFC 4627 states "The representation of numbers is similar to that used
in most programming languages."  It then defines syntax.  That is all.
RFC 7159 follows suit.

In JavaScript, 42 and 42.0 are the same thing, because JavaScript
provides just one numeric type: IEEE double precision.

In most programming languages, they are not, because most programming
languages provide distinct numerical types for integers and
floating-point numbers.  For instance, C makes 42 an int, and 42.0 a
double.  See also Java, Lisp, ...

JSON is *not* a subset of JavaScript!  Perhaps it was once intended to
be one, but if that was the case, then the job got botched pretty
comprehensively.  Here's another difference, for your entertainment:
https://writing.pupius.co.uk/json-js-42a28471221d

>                    So whatever interprets a JSON value semantically will
> just see something that is a number value and it should not be able to
> tell whether that number had a decimal point or not (except for guessing
> by looking whether there's a fractional part).

True if your dialect of JSON treats all numbers as double.  Not true for
our dialect of JSON.

> Therefore, if you reject a number based on the fact that it has a
> decimal point in it, that to me seems like syntax, not semantics.
>
> In any case, to me it's no different from discriminating between 42.0
> and 4.2e1 (which even in C is exactly the same value).

Yes, but 42 isn't.

>> We can of course make ABI more accepting.  However, messing with the QMP
>> ABI is *hairy*, and we should therefore mess with it only when we have a
>> damn good practical reason.  "It's not nice" ain't.
>
> That depends on who looks at it.  You don't think it's a good reason,
> OK.  I think it is.
>
> I hope you can excuse me for not having yet made my fair share of bad
> experiences with trying to fix things and thus breaking them even
> further.  I'm sure I'll get appropriately pessimistic over the coming
> years. :-)

People's lack of understanding how hard something is has always been a
major contributor to progress :)

That said, I think we got plenty of bigger fish to fry in QAPI/QMP-land.

>>>> Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
>>>> value is 'number':
>>>>
>>>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
>>>>     <- {"return": {}}
>>>>     -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
>>>>     <- {"return": {}}
>>>>
>>>> Don't blame me for the parts of QMP I inherited :)
>>>
>>> I sure don't.  But I am willing to start a discussion by calling that a
>>> bug. ;-)
>>>
>>> QNum has only been introduced recently.  Before, we had a hard split of
>>> QInt and QFloat.  So I'm not surprised that we haven't fixed everything yet.
>>>
>>> OTOH the introduction of QNum to me signals that we do want to fix this
>>> eventually.
>> 
>> QNum was introduced to get us unsigned numbers with the least possible
>> notational overhead.  It wasn't introduced to signal intent to redesign
>> QMP numbers.
>
> Again, that is very much not obvious from looking at QNum.  Why does it
> include floats then?  Because some basically integer values were
> represented as floats because they were supposed to be unsigned and did
> not fit into an int64_t?

QMP needs to bridge JSON to QAPI.  JSON has its (underspecified) JSON
number.  QAPI has integers and double.  The QMP designers chose to have
QMP accept any JSON number as double, but only numbers without decimal
point and exponent as integer.

This is implemented partly in the parser (which creates the approprate
QNum variant, see parse_literal()), and partly in qnum.c (where
qnum_get_double() accepts any variant).  Before qnum.c, the latter part
had to be done in every place that gets a double (at the time,
qobject_input_type_number() and qdict_get_double()).  One of the reasons
I like the QNum solution.

> I could understand that from a technical perspective, but it sounds more
> like we should have expanded QInt then to cover both signed and unsigned
> integers and then fixed places which tried to "abuse" QFloat for
> unsigned integers.

Marc-André first proposed a solution with separate QInt, QUInt, QFloat.
I asked him to explore QNum as well, and that one turned out nicely, so
we picked it.

Separate QInt (with signed and unsigned variant) and QFloat would've
been possible, too.

Here's how I like to think about QNum.

At the implementation level, QNum has int64_t, uint64_t and double
variants, and is a subtype of QObject.

At the conceptual level, we have a signed integer, an unsigned integer
and a floating-point type, all subtypes of a number type, which is a
subtype of a value type.

The fact that some subtypes get their own C type while others "only"
become variants is an implementation detail.

>>>>> Finally, I think it's rather pointless not to consider 42u and 42 the
>>>>> same value.  But since unsigned/signed are two different kinds of QNums
>>>>> already, we cannot consider them equal without considering 42.0 equal,
>>>>> too.
>>>>
>>>> Non sequitur.
>>>>
>>>>> Because of this, I have decided to continue to compare QNum values even
>>>>> if they are of a different kind.
>>>>
>>>> I think comparing signed and unsigned integer QNums is fair and
>>>> consistent with how the rest of our code works.
>>>
>>> I don't see how. doubles can represent different numbers than integers
>>> can. Signed integers can represent different numbers than unsigned can.
>> 
>> The only way to add unsigned integers without breaking QMP compatibility
>> is to make them interchangeable with signed integers.  That doesn't mean
>> you get to make floating-point numbers interchangeable with integers
>> now.
>
> Again, begs the question why QNum covers floating point numbers then and
> why this very fact is not documented in qnum.c.

What kind of documentation would you like to see?

>>> Sure, signed/unsigned makes less of a difference than having an exponent
>>> does.  But I don't agree we should make a difference when the only
>>> reason not to seems to be "qemu currently likes to make a difference in
>>> its interface, for historical reasons mainly" and "Do you really want to
>>> write this equality function?  It seems hard to get right".
>> 
>> "Because this is an interesting puzzle I'd love to solve" is wholly
>> insufficient reason to mess with QMP ABI.
>
> I don't see how I'm messing with the QMP ABI here, but with an
> s/QMP ABI/this/, I see your point.

Because the QMP ABI is in part implemented in qnum.c.  Change to qnum.c
has to consider its effect on the QMP ABI.  Counts as "messing" in my
book.  I could've explained this more clearly, I guess.

>>                                            It's also an insufficient
>> reason to add "interesting" code for me to maintain.
>
> Now this is a point I can fully understand and agree on.
>
>>> For the record, I could have lived with the old separation into QInt and
>>> QFloat.  But now we do have a common QNum and I think the idea behind is
>>> is to have a uniform opaque interface.
>> 
>> Nope, the idea is to get unsigned integers through QMP with the least
>> notational overhead.
>
> (Again, why include floats, then?)
>
>>>> Comparing integer and floating QNums isn't.  It's also a can of worms.
>>>> Are you sure we *need* to open that can *now*?
>>>
>>> Sure?  No.  Do I want to?  I guess so.
>>>
>>>> Are you sure a simple, stupid eql-like comparison won't do *for now*?
>>>> YAGNI!
>>>
>>> But I want it.  I think the current behavior your demonstrated above is
>>> a bug and I don't really want to continue to follow it.
>> 
>> Feel free to call the current behavior a bug.  But it's a design bug
>> then.  Fixing design bugs in ABIs is somewhere between hard and
>> impractical.  I do not think this one is worth your while or mine.
>
> Technical question: How is this an ABI and not an API?  Making QNum
> replace QInt and QFloat was messing with the ABI.

If QEMU provided a C ABI that exposed QInt/QFloat/QNum, then the change
to QNum would've messed with that C ABI.  QEMU does not.

An ABI it does provide is the QMP ABI.  The change to QNum should be
invisible there.  We reviewed it carefully in that regard.

>                                                    Now, making QNum
> behave as both depending on what is asked for is just an API change,
> isn't it?

I suspect it would make QMP accept 42.0 as valid integer.  That would be
an ABI change.  I wouldn't bet on this being the only one without
careful review.

> Also, I still don't see how just converting every JSON number into a
> QNum and then making QNum return a valid integer or float depending on
> who's asking would be hard or impractical.

It's certainly possible.  It's just a lot more work than hacking up the
code for it.  The more work something takes, the stronger its
justification needs to be.

> (But really, don't bother to reply.  I pretty much know I'm overlooking
> a lot here and this is just my naive standpoint.  Again, though, maybe
> there should be documentation in qnum.c about this.)
>
>>> All you have really convinced me to do is to add another patch which
>>> smacks a warning on qnum_get_int(), and maybe even a TODO that it should
>>> convert doubles to integers *if possible*.
>>>
>>> (And the "if possible" just means that you cannot convert values which
>>> are out of bounds or NaN.  Fractional parts may not even matter much --
>>> I mean, we do happily convert integers to doubles and rounding that way
>>> is implementation-defined.)
>> 
>> Always try the stupidest solution that could possibly work first.
>> Unless I misunderstand your use case, a simple & stupid
>> qobject_is_equal() would do.  So let's try that first.
> Honestly, I pretty much hate it.  But I can't say I disagree with your
> most important points (it'd be useless, it'd be overly complicated,
> you'd have to maintain something you don't want), so yep, will do.
>
>> Adding capability to compare signed and unsigned integers should still
>> be fairly simple.  I'd be willing to consider it.
>
> Thanks for bearing with me. :-)

Thank you for speaking your mind, and for hearing me out!

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

[Max Reitz]

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

On 2017-07-11 13:33, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
>> least not right now and in the foreseeable future).
>> You're the maintainer, so you decide, so I'll go along with it. :-)
>>
>> Now, let's follow up with my therefore rather useless commentary:
>>
>> (Feel free to disregard, because honestly, I can see how replying to
>> most of the points I'm asking isn't really worth the time...)
> 
> When I use the authority entrusted to maintainers, I feel obliged to at
> least explain my reasoning.  Besides, putting my reasoning in words
> tends to lead me to new insights.

And I am indeed very grateful for that. :-)

>> On 2017-07-10 11:17, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> On 2017-07-06 16:30, Markus Armbruster wrote:

[...]

>>> The only way to add unsigned integers without breaking QMP compatibility
>>> is to make them interchangeable with signed integers.  That doesn't mean
>>> you get to make floating-point numbers interchangeable with integers
>>> now.
>>
>> Again, begs the question why QNum covers floating point numbers then and
>> why this very fact is not documented in qnum.c.
> 
> What kind of documentation would you like to see?

It would be good to note that the QNum type is not meant to be a
completely uniform way to handle JSON numbers (e.g. if the user provides
something with a decimal point but you need an integer, QNum will not do
that conversion for you).

It is (English indirect speech is broken badly) just meant to
encapsulate the different variants a number can be represented in, but
you're still generally supposed to read it out the way it was put in
(exceptions apply, see signed/unsigned and qnum_get_double()).

Max


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

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-07-11 13:33, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
>>> least not right now and in the foreseeable future).
>>> You're the maintainer, so you decide, so I'll go along with it. :-)
>>>
>>> Now, let's follow up with my therefore rather useless commentary:
>>>
>>> (Feel free to disregard, because honestly, I can see how replying to
>>> most of the points I'm asking isn't really worth the time...)
>> 
>> When I use the authority entrusted to maintainers, I feel obliged to at
>> least explain my reasoning.  Besides, putting my reasoning in words
>> tends to lead me to new insights.
>
> And I am indeed very grateful for that. :-)
>
>>> On 2017-07-10 11:17, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> On 2017-07-06 16:30, Markus Armbruster wrote:
>
> [...]
>
>>>> The only way to add unsigned integers without breaking QMP compatibility
>>>> is to make them interchangeable with signed integers.  That doesn't mean
>>>> you get to make floating-point numbers interchangeable with integers
>>>> now.
>>>
>>> Again, begs the question why QNum covers floating point numbers then and
>>> why this very fact is not documented in qnum.c.
>> 
>> What kind of documentation would you like to see?
>
> It would be good to note that the QNum type is not meant to be a
> completely uniform way to handle JSON numbers (e.g. if the user provides
> something with a decimal point but you need an integer, QNum will not do
> that conversion for you).
>
> It is (English indirect speech is broken badly) just meant to
> encapsulate the different variants a number can be represented in, but
> you're still generally supposed to read it out the way it was put in
> (exceptions apply, see signed/unsigned and qnum_get_double()).

Can we distill this into text that could become an actual patch?  Let me
try.

    QNum encapsulates how our dialect of JSON fills in the blanks left
    by the JSON specification (RFC 7159) regarding numbers.

    Conceptually, we treat number as an abstract type with three
    concrete subtypes: floating-point, signed integer, unsigned integer.
    QNum implements this a discriminated union of double, int64_t,
    uint64_t.

    The JSON parser picks the subtype as follows.  If the number has a
    decimal point or an exponent, it is floating-point.  Else if it fits
    into int64_t, it's signed integer.  Else if it first into uint64_t,
    it's unsigned integer.  Else it's floating-point.

    Any number can serve as double: qnum_get_double() converts under the
    hood.

    An integer can serve as signed / unsigned integer as long as it is
    in range: qnum_get_try_int() / qnum_get_try_uint() check range and
    convert under the hood.

What do you think?

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

[Max Reitz]

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

On 2017-08-14 11:07, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2017-07-11 13:33, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
>>>> least not right now and in the foreseeable future).
>>>> You're the maintainer, so you decide, so I'll go along with it. :-)
>>>>
>>>> Now, let's follow up with my therefore rather useless commentary:
>>>>
>>>> (Feel free to disregard, because honestly, I can see how replying to
>>>> most of the points I'm asking isn't really worth the time...)
>>>
>>> When I use the authority entrusted to maintainers, I feel obliged to at
>>> least explain my reasoning.  Besides, putting my reasoning in words
>>> tends to lead me to new insights.
>>
>> And I am indeed very grateful for that. :-)
>>
>>>> On 2017-07-10 11:17, Markus Armbruster wrote:
>>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>>
>>>>>> On 2017-07-06 16:30, Markus Armbruster wrote:
>>
>> [...]
>>
>>>>> The only way to add unsigned integers without breaking QMP compatibility
>>>>> is to make them interchangeable with signed integers.  That doesn't mean
>>>>> you get to make floating-point numbers interchangeable with integers
>>>>> now.
>>>>
>>>> Again, begs the question why QNum covers floating point numbers then and
>>>> why this very fact is not documented in qnum.c.
>>>
>>> What kind of documentation would you like to see?
>>
>> It would be good to note that the QNum type is not meant to be a
>> completely uniform way to handle JSON numbers (e.g. if the user provides
>> something with a decimal point but you need an integer, QNum will not do
>> that conversion for you).
>>
>> It is (English indirect speech is broken badly) just meant to
>> encapsulate the different variants a number can be represented in, but
>> you're still generally supposed to read it out the way it was put in
>> (exceptions apply, see signed/unsigned and qnum_get_double()).
> 
> Can we distill this into text that could become an actual patch?  Let me
> try.
> 
>     QNum encapsulates how our dialect of JSON fills in the blanks left
>     by the JSON specification (RFC 7159) regarding numbers.
> 
>     Conceptually, we treat number as an abstract type with three
>     concrete subtypes: floating-point, signed integer, unsigned integer.
>     QNum implements this a discriminated union of double, int64_t,
>     uint64_t.
> 
>     The JSON parser picks the subtype as follows.  If the number has a
>     decimal point or an exponent, it is floating-point.  Else if it fits
>     into int64_t, it's signed integer.  Else if it first into uint64_t,
>     it's unsigned integer.  Else it's floating-point.
> 
>     Any number can serve as double: qnum_get_double() converts under the
>     hood.
> 
>     An integer can serve as signed / unsigned integer as long as it is
>     in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>     convert under the hood.
> 
> What do you think?

Sounds very good to me, thanks!

Max


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