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

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


v2:
- Add comments detailing when QDicts and QLists are considered equal
  [Kevin]


git-backport-diff against v1:

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/4:[----] [--] 'qapi/qnull: Add own header'
002/4:[0009] [FC] 'qapi: Add qobject_is_equal()'
003/4:[----] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
004/4:[----] [--] 'iotests: Add test for non-string option reopening'


Max Reitz (4):
  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

 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qfloat.h  |  1 +
 include/qapi/qmp/qint.h    |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   | 28 ++++++++++++++++++++++++++++
 include/qapi/qmp/qobject.h | 17 +++++++++--------
 include/qapi/qmp/qstring.h |  1 +
 include/qapi/qmp/types.h   |  1 +
 block.c                    | 15 +++------------
 qobject/qbool.c            |  8 ++++++++
 qobject/qdict.c            | 28 ++++++++++++++++++++++++++++
 qobject/qfloat.c           |  8 ++++++++
 qobject/qint.c             |  8 ++++++++
 qobject/qlist.c            | 30 ++++++++++++++++++++++++++++++
 qobject/qnull.c            |  6 ++++++
 qobject/qobject.c          | 30 ++++++++++++++++++++++++++++++
 qobject/qstring.c          |  9 +++++++++
 target/i386/cpu.c          |  6 +-----
 tests/check-qnull.c        |  2 +-
 tests/qemu-iotests/133     |  9 +++++++++
 tests/qemu-iotests/133.out |  5 +++++
 22 files changed, 190 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Max Reitz]

Reviewed-by: Kevin Wolf <kwolf@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            |  1 +
 target/i386/cpu.c          |  6 +-----
 tests/check-qnull.c        |  2 +-
 6 files changed, 30 insertions(+), 14 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..69555ac
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,26 @@
+/*
+ * QNull Module
+ *
+ * Copyright (C) 2009, 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@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 27cfbd8..4c87182 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -20,5 +20,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..b3cc85e 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -13,6 +13,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/target/i386/cpu.c b/target/i386/cpu.c
index b2b1d20..f118a54 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -29,11 +29,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qfloat.h"
+#include "qapi/qmp/types.h"
 
 #include "qapi-types.h"
 #include "qapi-visit.h"
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 v2 2/4] qapi: Add qobject_is_equal()

[Max Reitz]

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

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qfloat.h  |  1 +
 include/qapi/qmp/qint.h    |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   |  2 ++
 include/qapi/qmp/qobject.h |  9 +++++++++
 include/qapi/qmp/qstring.h |  1 +
 qobject/qbool.c            |  8 ++++++++
 qobject/qdict.c            | 28 ++++++++++++++++++++++++++++
 qobject/qfloat.c           |  8 ++++++++
 qobject/qint.c             |  8 ++++++++
 qobject/qlist.c            | 30 ++++++++++++++++++++++++++++++
 qobject/qnull.c            |  5 +++++
 qobject/qobject.c          | 30 ++++++++++++++++++++++++++++++
 qobject/qstring.c          |  9 +++++++++
 16 files changed, 143 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 188440a..134a526 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -41,6 +41,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/qfloat.h b/include/qapi/qmp/qfloat.h
index b5d1583..318e91e 100644
--- a/include/qapi/qmp/qfloat.h
+++ b/include/qapi/qmp/qfloat.h
@@ -24,6 +24,7 @@ typedef struct QFloat {
 QFloat *qfloat_from_double(double value);
 double qfloat_get_double(const QFloat *qi);
 QFloat *qobject_to_qfloat(const QObject *obj);
+bool qfloat_is_equal(const QObject *x, const QObject *y);
 void qfloat_destroy_obj(QObject *obj);
 
 #endif /* QFLOAT_H */
diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
index 3aaff76..20975da 100644
--- a/include/qapi/qmp/qint.h
+++ b/include/qapi/qmp/qint.h
@@ -23,6 +23,7 @@ typedef struct QInt {
 QInt *qint_from_int(int64_t value);
 int64_t qint_get_int(const QInt *qi);
 QInt *qobject_to_qint(const QObject *obj);
+bool qint_is_equal(const QObject *x, const QObject *y);
 void qint_destroy_obj(QObject *obj);
 
 #endif /* QINT_H */
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 5dc4ed9..4380a5b 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -57,6 +57,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 69555ac..9299683 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/qobject.h b/include/qapi/qmp/qobject.h
index ef1d1a9..cac72e3 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(): Returns whether the two objects are equal.
+ *
+ * Any of the pointers may be NULL; will return true if both are. Will 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..f1d1719 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
 }
 
 /**
+ * qbool_is_equal(): Tests 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 88e2ecd..ca919bc 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -410,6 +410,34 @@ void qdict_del(QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_is_equal(): Tests 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), *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/qfloat.c b/qobject/qfloat.c
index d5da847..291ecc4 100644
--- a/qobject/qfloat.c
+++ b/qobject/qfloat.c
@@ -52,6 +52,14 @@ QFloat *qobject_to_qfloat(const QObject *obj)
 }
 
 /**
+ * qfloat_is_equal(): Tests whether the two QFloats are equal
+ */
+bool qfloat_is_equal(const QObject *x, const QObject *y)
+{
+    return qobject_to_qfloat(x)->value == qobject_to_qfloat(y)->value;
+}
+
+/**
  * qfloat_destroy_obj(): Free all memory allocated by a
  * QFloat object
  */
diff --git a/qobject/qint.c b/qobject/qint.c
index d7d1b30..f638cb7 100644
--- a/qobject/qint.c
+++ b/qobject/qint.c
@@ -51,6 +51,14 @@ QInt *qobject_to_qint(const QObject *obj)
 }
 
 /**
+ * qint_is_equal(): Tests whether the two QInts are equal
+ */
+bool qint_is_equal(const QObject *x, const QObject *y)
+{
+    return qobject_to_qint(x)->value == qobject_to_qint(y)->value;
+}
+
+/**
  * qint_destroy_obj(): Free all memory allocated by a
  * QInt object
  */
diff --git a/qobject/qlist.c b/qobject/qlist.c
index 86b60cb..652fc53 100644
--- a/qobject/qlist.c
+++ b/qobject/qlist.c
@@ -140,6 +140,36 @@ QList *qobject_to_qlist(const QObject *obj)
 }
 
 /**
+ * qlist_is_equal(): Tests 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), *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 b3cc85e..af5453d 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -19,3 +19,8 @@ QObject qnull_ = {
     .type = QTYPE_QNULL,
     .refcnt = 1,
 };
+
+bool qnull_is_equal(const QObject *x, const QObject *y)
+{
+    return true;
+}
diff --git a/qobject/qobject.c b/qobject/qobject.c
index fe4fa10..af2869a 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -28,3 +28,33 @@ 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_QINT] = qint_is_equal,
+    [QTYPE_QSTRING] = qstring_is_equal,
+    [QTYPE_QDICT] = qdict_is_equal,
+    [QTYPE_QLIST] = qlist_is_equal,
+    [QTYPE_QFLOAT] = qfloat_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..a2b51fa 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -129,6 +129,15 @@ const char *qstring_get_str(const QString *qstring)
 }
 
 /**
+ * qstring_is_equal(): Tests 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 v2 3/4] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

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

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

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index fa1d06d..23424d5 100644
--- a/block.c
+++ b/block.c
@@ -2950,19 +2950,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         const QDictEntry *entry = qdict_first(reopen_state->options);
 
         do {
-            QString *new_obj = qobject_to_qstring(entry->value);
-            const char *new = qstring_get_str(new_obj);
-            /*
-             * Caution: while qdict_get_try_str() is fine, getting
-             * non-string types would require more care.  When
-             * bs->options come from -blockdev or blockdev_add, its
-             * members are typed according to the QAPI schema, but
-             * when they come from -drive, they're all QString.
-             */
-            const char *old = qdict_get_try_str(reopen_state->bs->options,
-                                                entry->key);
+            QObject *new = entry->value;
+            QObject *old = qdict_get(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 v2 4/4] 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

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Currently, bdrv_reopen_prepare() assumes that all BDS options are
> strings. However, this is not the case if the BDS has been created
> through the json: pseudo-protocol or blockdev-add.
>
> Note that the user-invokable reopen command is an HMP command, so you
> can only specify strings there. Therefore, specifying a non-string
> option with the "same" value as it was when originally created will now
> return an error because the values are supposedly similar (and there is
> no way for the user to circumvent this but to just not specify the
> option again -- however, this is still strictly better than just
> crashing).
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index fa1d06d..23424d5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2950,19 +2950,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          const QDictEntry *entry = qdict_first(reopen_state->options);
>  
>          do {
> -            QString *new_obj = qobject_to_qstring(entry->value);
> -            const char *new = qstring_get_str(new_obj);
> -            /*
> -             * Caution: while qdict_get_try_str() is fine, getting
> -             * non-string types would require more care.  When
> -             * bs->options come from -blockdev or blockdev_add, its
> -             * members are typed according to the QAPI schema, but
> -             * when they come from -drive, they're all QString.
> -             */

Your commit message makes me suspect this comment still applies in some
form.  Does it?

> -            const char *old = qdict_get_try_str(reopen_state->bs->options,
> -                                                entry->key);
> +            QObject *new = entry->value;
> +            QObject *old = qdict_get(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;

Re: [Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Reviewed-by: Kevin Wolf <kwolf@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            |  1 +
>  target/i386/cpu.c          |  6 +-----
>  tests/check-qnull.c        |  2 +-
>  6 files changed, 30 insertions(+), 14 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..69555ac
> --- /dev/null
> +++ b/include/qapi/qmp/qnull.h
> @@ -0,0 +1,26 @@
> +/*
> + * QNull Module
> + *
> + * Copyright (C) 2009, 2017 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <lcapitulino@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.

Copy the boilerplate from qnull.c instead, factual correctness.

> + */
> +
> +#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 */

Meh, another tiny header.  Are our compiles too fast?

> 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 27cfbd8..4c87182 100644
> --- a/include/qapi/qmp/types.h
> +++ b/include/qapi/qmp/types.h
> @@ -20,5 +20,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..b3cc85e 100644
> --- a/qobject/qnull.c
> +++ b/qobject/qnull.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qapi/qmp/qobject.h"

Let's drop this include, like you do in check-qnull.c below.

> +#include "qapi/qmp/qnull.h"
>  
>  QObject qnull_ = {
>      .type = QTYPE_QNULL,
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2b1d20..f118a54 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -29,11 +29,7 @@
>  #include "qemu/option.h"
>  #include "qemu/config-file.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qapi/qmp/qstring.h"
> -#include "qapi/qmp/qdict.h"
> -#include "qapi/qmp/qbool.h"
> -#include "qapi/qmp/qint.h"
> -#include "qapi/qmp/qfloat.h"
> +#include "qapi/qmp/types.h"

qapi/qmp/types.h is a lazy way to increase compile times by including
more than you need.  One day I'll kill it.  Until then, I tolerate it in
.c, but not in .h.

>  
>  #include "qapi-types.h"
>  #include "qapi-visit.h"
> 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"

With the boilerplate corrected and the superfluous include in qnull.c
deleted
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/4] 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>
> ---
>  include/qapi/qmp/qbool.h   |  1 +
>  include/qapi/qmp/qdict.h   |  1 +
>  include/qapi/qmp/qfloat.h  |  1 +
>  include/qapi/qmp/qint.h    |  1 +
>  include/qapi/qmp/qlist.h   |  1 +
>  include/qapi/qmp/qnull.h   |  2 ++
>  include/qapi/qmp/qobject.h |  9 +++++++++
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qbool.c            |  8 ++++++++
>  qobject/qdict.c            | 28 ++++++++++++++++++++++++++++
>  qobject/qfloat.c           |  8 ++++++++
>  qobject/qint.c             |  8 ++++++++
>  qobject/qlist.c            | 30 ++++++++++++++++++++++++++++++
>  qobject/qnull.c            |  5 +++++
>  qobject/qobject.c          | 30 ++++++++++++++++++++++++++++++
>  qobject/qstring.c          |  9 +++++++++
>  16 files changed, 143 insertions(+)

No unit test?

> 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 188440a..134a526 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -41,6 +41,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/qfloat.h b/include/qapi/qmp/qfloat.h
> index b5d1583..318e91e 100644
> --- a/include/qapi/qmp/qfloat.h
> +++ b/include/qapi/qmp/qfloat.h
> @@ -24,6 +24,7 @@ typedef struct QFloat {
>  QFloat *qfloat_from_double(double value);
>  double qfloat_get_double(const QFloat *qi);
>  QFloat *qobject_to_qfloat(const QObject *obj);
> +bool qfloat_is_equal(const QObject *x, const QObject *y);
>  void qfloat_destroy_obj(QObject *obj);
>  
>  #endif /* QFLOAT_H */
> diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
> index 3aaff76..20975da 100644
> --- a/include/qapi/qmp/qint.h
> +++ b/include/qapi/qmp/qint.h
> @@ -23,6 +23,7 @@ typedef struct QInt {
>  QInt *qint_from_int(int64_t value);
>  int64_t qint_get_int(const QInt *qi);
>  QInt *qobject_to_qint(const QObject *obj);
> +bool qint_is_equal(const QObject *x, const QObject *y);
>  void qint_destroy_obj(QObject *obj);
>  
>  #endif /* QINT_H */
> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
> index 5dc4ed9..4380a5b 100644
> --- a/include/qapi/qmp/qlist.h
> +++ b/include/qapi/qmp/qlist.h
> @@ -57,6 +57,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 69555ac..9299683 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/qobject.h b/include/qapi/qmp/qobject.h
> index ef1d1a9..cac72e3 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(): Returns whether the two objects are equal.

Imperative mood, please: Return whether...

> + *
> + * Any of the pointers may be NULL; will return true if both are. Will always
> + * return false if only one is (therefore a QNull object is not considered equal
> + * to a NULL pointer).
> + */

Humor me: wrap comment lines around column 70, and put two spaces
between sentences.

Same for the other function comments.

> +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..f1d1719 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
>  }
>  
>  /**
> + * qbool_is_equal(): Tests whether the two QBools are equal

Return whether ...

> + */
> +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 88e2ecd..ca919bc 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -410,6 +410,34 @@ void qdict_del(QDict *qdict, const char *key)
>  }
>  
>  /**
> + * qdict_is_equal(): Tests 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), *dict_y = qobject_to_qdict(y);

I'm fine with multiple initializers in one declaration, but I think this
one would look tidier as

       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/qfloat.c b/qobject/qfloat.c
> index d5da847..291ecc4 100644
> --- a/qobject/qfloat.c
> +++ b/qobject/qfloat.c
> @@ -52,6 +52,14 @@ QFloat *qobject_to_qfloat(const QObject *obj)
>  }
>  
>  /**
> + * qfloat_is_equal(): Tests whether the two QFloats are equal
> + */
> +bool qfloat_is_equal(const QObject *x, const QObject *y)
> +{
> +    return qobject_to_qfloat(x)->value == qobject_to_qfloat(y)->value;
> +}
> +
> +/**
>   * qfloat_destroy_obj(): Free all memory allocated by a
>   * QFloat object
>   */
> diff --git a/qobject/qint.c b/qobject/qint.c
> index d7d1b30..f638cb7 100644
> --- a/qobject/qint.c
> +++ b/qobject/qint.c
> @@ -51,6 +51,14 @@ QInt *qobject_to_qint(const QObject *obj)
>  }
>  
>  /**
> + * qint_is_equal(): Tests whether the two QInts are equal
> + */
> +bool qint_is_equal(const QObject *x, const QObject *y)
> +{
> +    return qobject_to_qint(x)->value == qobject_to_qint(y)->value;
> +}
> +
> +/**
>   * qint_destroy_obj(): Free all memory allocated by a
>   * QInt object
>   */
> diff --git a/qobject/qlist.c b/qobject/qlist.c
> index 86b60cb..652fc53 100644
> --- a/qobject/qlist.c
> +++ b/qobject/qlist.c
> @@ -140,6 +140,36 @@ QList *qobject_to_qlist(const QObject *obj)
>  }
>  
>  /**
> + * qlist_is_equal(): Tests 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), *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 b3cc85e..af5453d 100644
> --- a/qobject/qnull.c
> +++ b/qobject/qnull.c
> @@ -19,3 +19,8 @@ QObject qnull_ = {
>      .type = QTYPE_QNULL,
>      .refcnt = 1,
>  };
> +
> +bool qnull_is_equal(const QObject *x, const QObject *y)
> +{
> +    return true;
> +}
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> index fe4fa10..af2869a 100644
> --- a/qobject/qobject.c
> +++ b/qobject/qobject.c
> @@ -28,3 +28,33 @@ 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_QINT] = qint_is_equal,
> +    [QTYPE_QSTRING] = qstring_is_equal,
> +    [QTYPE_QDICT] = qdict_is_equal,
> +    [QTYPE_QLIST] = qlist_is_equal,
> +    [QTYPE_QFLOAT] = qfloat_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..a2b51fa 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -129,6 +129,15 @@ const char *qstring_get_str(const QString *qstring)
>  }
>  
>  /**
> + * qstring_is_equal(): Tests 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
>   */

Address my nitpicks, and either provide a unit test or convince me it's
not worth the trouble, and you may add
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Max Reitz]

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

On 2017-06-21 18:06, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Currently, bdrv_reopen_prepare() assumes that all BDS options are
>> strings. However, this is not the case if the BDS has been created
>> through the json: pseudo-protocol or blockdev-add.
>>
>> Note that the user-invokable reopen command is an HMP command, so you
>> can only specify strings there. Therefore, specifying a non-string
>> option with the "same" value as it was when originally created will now
>> return an error because the values are supposedly similar (and there is
>> no way for the user to circumvent this but to just not specify the
>> option again -- however, this is still strictly better than just
>> crashing).
>>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 15 +++------------
>>  1 file changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index fa1d06d..23424d5 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2950,19 +2950,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>          const QDictEntry *entry = qdict_first(reopen_state->options);
>>  
>>          do {
>> -            QString *new_obj = qobject_to_qstring(entry->value);
>> -            const char *new = qstring_get_str(new_obj);
>> -            /*
>> -             * Caution: while qdict_get_try_str() is fine, getting
>> -             * non-string types would require more care.  When
>> -             * bs->options come from -blockdev or blockdev_add, its
>> -             * members are typed according to the QAPI schema, but
>> -             * when they come from -drive, they're all QString.
>> -             */
> 
> Your commit message makes me suspect this comment still applies in some
> form.  Does it?

The only thing that I can think of that may be applicable is what I
wrote in the commit message; this fails now:

$ qemu-io -c 'reopen -o size=65536' \
    "json:{'driver':'null-co','size':65536}"
Cannot change the option 'size'

As I wrote in the commit message, though, I don't think this is too bad.
First, before, it just crashed, so this definitely is better behavior.

Secondly, I think this is just good for convenience; it allows the user
to specify an option (to "change" it) even if the block driver doesn't
support changing it. If it actually has not change, this is supposed to
be handled gracefully. But sometimes we cannot easily handle it, so...
We just give an error.

I suspect that at some point we want to fix this by having everything
correctly typed internally...? Until then, in my opinion, we can just
not provide this feature.

However, I should have probably not just deleted the comment and hoped
everyone can find the necessary information through git-blame. I should
leave a comment about this here.


Or we do make an effort to provide this test-unchanged functionality for
every case, in which case we would have to convert either string options
to the correct type (according to the schema) here (but if that were so
simple, we could do that centrally in bdrv_open() already, right?) or
convert typed options to strings and compare them -- but since there are
probably multiple different strings that can mean the same value for
whatever type the option has, this won't work in every case either.

So I'm for leaving a comment and leaving this not quite as it should be
until we have fixed all of the block layer (O:-)). Maybe you have a
better idea though, which would be great.

Max

>> -            const char *old = qdict_get_try_str(reopen_state->bs->options,
>> -                                                entry->key);
>> +            QObject *new = entry->value;
>> +            QObject *old = qdict_get(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;



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

Re: [Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Max Reitz]

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

On 2017-06-21 18:24, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Reviewed-by: Kevin Wolf <kwolf@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            |  1 +
>>  target/i386/cpu.c          |  6 +-----
>>  tests/check-qnull.c        |  2 +-
>>  6 files changed, 30 insertions(+), 14 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..69555ac
>> --- /dev/null
>> +++ b/include/qapi/qmp/qnull.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * QNull Module
>> + *
>> + * Copyright (C) 2009, 2017 Red Hat Inc.
>> + *
>> + * Authors:
>> + *  Luiz Capitulino <lcapitulino@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.
> 
> Copy the boilerplate from qnull.c instead, factual correctness.

Sorry, will do.

>> + */
>> +
>> +#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 */
> 
> Meh, another tiny header.  Are our compiles too fast?

For me, testing takes longer, so from my POV they're fast enough. ;-)

You can always merge all the QObject types into a single header; but as
the code is organized now, I think QNull should have its own header.

>> 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 27cfbd8..4c87182 100644
>> --- a/include/qapi/qmp/types.h
>> +++ b/include/qapi/qmp/types.h
>> @@ -20,5 +20,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..b3cc85e 100644
>> --- a/qobject/qnull.c
>> +++ b/qobject/qnull.c
>> @@ -13,6 +13,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qapi/qmp/qobject.h"
> 
> Let's drop this include, like you do in check-qnull.c below.

I don't even know why I've dropped it there, must have been an accident.
But, well, I don't mind having more happy little accidents.

>> +#include "qapi/qmp/qnull.h"
>>  
>>  QObject qnull_ = {
>>      .type = QTYPE_QNULL,
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index b2b1d20..f118a54 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -29,11 +29,7 @@
>>  #include "qemu/option.h"
>>  #include "qemu/config-file.h"
>>  #include "qapi/qmp/qerror.h"
>> -#include "qapi/qmp/qstring.h"
>> -#include "qapi/qmp/qdict.h"
>> -#include "qapi/qmp/qbool.h"
>> -#include "qapi/qmp/qint.h"
>> -#include "qapi/qmp/qfloat.h"
>> +#include "qapi/qmp/types.h"
> 
> qapi/qmp/types.h is a lazy way to increase compile times by including
> more than you need.  One day I'll kill it.  Until then, I tolerate it in
> .c, but not in .h.

First, I think the compiler has enough work to do on i386/cpu.c as it
is, so it won't matter much here.

Secondly, I'll gladly take a qapi/qmp/primitive-types.h; but I don't see
the point of including six different headers and keeping track of which
primitives type you plan to use.

So if you want to merge them all into one, go for out, sounds very
reasonable to me.

>>  
>>  #include "qapi-types.h"
>>  #include "qapi-visit.h"
>> 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"
> 
> With the boilerplate corrected and the superfluous include in qnull.c
> deleted
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Will do, thanks!

Max


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

Re: [Qemu-devel] [PATCH v2 2/4] qapi: Add qobject_is_equal()

[Max Reitz]

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

On 2017-06-21 18:43, 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>
>> ---
>>  include/qapi/qmp/qbool.h   |  1 +
>>  include/qapi/qmp/qdict.h   |  1 +
>>  include/qapi/qmp/qfloat.h  |  1 +
>>  include/qapi/qmp/qint.h    |  1 +
>>  include/qapi/qmp/qlist.h   |  1 +
>>  include/qapi/qmp/qnull.h   |  2 ++
>>  include/qapi/qmp/qobject.h |  9 +++++++++
>>  include/qapi/qmp/qstring.h |  1 +
>>  qobject/qbool.c            |  8 ++++++++
>>  qobject/qdict.c            | 28 ++++++++++++++++++++++++++++
>>  qobject/qfloat.c           |  8 ++++++++
>>  qobject/qint.c             |  8 ++++++++
>>  qobject/qlist.c            | 30 ++++++++++++++++++++++++++++++
>>  qobject/qnull.c            |  5 +++++
>>  qobject/qobject.c          | 30 ++++++++++++++++++++++++++++++
>>  qobject/qstring.c          |  9 +++++++++
>>  16 files changed, 143 insertions(+)
> 
> No unit test?

*cough*

>> 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 188440a..134a526 100644
>> --- a/include/qapi/qmp/qdict.h
>> +++ b/include/qapi/qmp/qdict.h
>> @@ -41,6 +41,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/qfloat.h b/include/qapi/qmp/qfloat.h
>> index b5d1583..318e91e 100644
>> --- a/include/qapi/qmp/qfloat.h
>> +++ b/include/qapi/qmp/qfloat.h
>> @@ -24,6 +24,7 @@ typedef struct QFloat {
>>  QFloat *qfloat_from_double(double value);
>>  double qfloat_get_double(const QFloat *qi);
>>  QFloat *qobject_to_qfloat(const QObject *obj);
>> +bool qfloat_is_equal(const QObject *x, const QObject *y);
>>  void qfloat_destroy_obj(QObject *obj);
>>  
>>  #endif /* QFLOAT_H */
>> diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
>> index 3aaff76..20975da 100644
>> --- a/include/qapi/qmp/qint.h
>> +++ b/include/qapi/qmp/qint.h
>> @@ -23,6 +23,7 @@ typedef struct QInt {
>>  QInt *qint_from_int(int64_t value);
>>  int64_t qint_get_int(const QInt *qi);
>>  QInt *qobject_to_qint(const QObject *obj);
>> +bool qint_is_equal(const QObject *x, const QObject *y);
>>  void qint_destroy_obj(QObject *obj);
>>  
>>  #endif /* QINT_H */
>> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
>> index 5dc4ed9..4380a5b 100644
>> --- a/include/qapi/qmp/qlist.h
>> +++ b/include/qapi/qmp/qlist.h
>> @@ -57,6 +57,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 69555ac..9299683 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/qobject.h b/include/qapi/qmp/qobject.h
>> index ef1d1a9..cac72e3 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(): Returns whether the two objects are equal.
> 
> Imperative mood, please: Return whether...

OK, will do here and everywhere else.

>> + *
>> + * Any of the pointers may be NULL; will return true if both are. Will always
>> + * return false if only one is (therefore a QNull object is not considered equal
>> + * to a NULL pointer).
>> + */
> 
> Humor me: wrap comment lines around column 70, and put two spaces
> between sentences.

Do I hear "one checkpatch.pl per subsystem"?

Yes, I know, there's no point to. Why should anyone not break comments
after 70 lines and not use two spaces after full stops? O:-)

> Same for the other function comments.

Sure, will do.

>> +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..f1d1719 100644
>> --- a/qobject/qbool.c
>> +++ b/qobject/qbool.c
>> @@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qbool_is_equal(): Tests whether the two QBools are equal
> 
> Return whether ...
> 
>> + */
>> +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 88e2ecd..ca919bc 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -410,6 +410,34 @@ void qdict_del(QDict *qdict, const char *key)
>>  }
>>  
>>  /**
>> + * qdict_is_equal(): Tests 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), *dict_y = qobject_to_qdict(y);
> 
> I'm fine with multiple initializers in one declaration, but I think this
> one would look tidier as
> 
>        const QDict *dict_x = qobject_to_qdict(x);
>        const QDict *dict_y = qobject_to_qdict(y);

I don't mind.

>> +    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/qfloat.c b/qobject/qfloat.c
>> index d5da847..291ecc4 100644
>> --- a/qobject/qfloat.c
>> +++ b/qobject/qfloat.c
>> @@ -52,6 +52,14 @@ QFloat *qobject_to_qfloat(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qfloat_is_equal(): Tests whether the two QFloats are equal
>> + */
>> +bool qfloat_is_equal(const QObject *x, const QObject *y)
>> +{
>> +    return qobject_to_qfloat(x)->value == qobject_to_qfloat(y)->value;
>> +}
>> +
>> +/**
>>   * qfloat_destroy_obj(): Free all memory allocated by a
>>   * QFloat object
>>   */
>> diff --git a/qobject/qint.c b/qobject/qint.c
>> index d7d1b30..f638cb7 100644
>> --- a/qobject/qint.c
>> +++ b/qobject/qint.c
>> @@ -51,6 +51,14 @@ QInt *qobject_to_qint(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qint_is_equal(): Tests whether the two QInts are equal
>> + */
>> +bool qint_is_equal(const QObject *x, const QObject *y)
>> +{
>> +    return qobject_to_qint(x)->value == qobject_to_qint(y)->value;
>> +}
>> +
>> +/**
>>   * qint_destroy_obj(): Free all memory allocated by a
>>   * QInt object
>>   */
>> diff --git a/qobject/qlist.c b/qobject/qlist.c
>> index 86b60cb..652fc53 100644
>> --- a/qobject/qlist.c
>> +++ b/qobject/qlist.c
>> @@ -140,6 +140,36 @@ QList *qobject_to_qlist(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qlist_is_equal(): Tests 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), *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 b3cc85e..af5453d 100644
>> --- a/qobject/qnull.c
>> +++ b/qobject/qnull.c
>> @@ -19,3 +19,8 @@ QObject qnull_ = {
>>      .type = QTYPE_QNULL,
>>      .refcnt = 1,
>>  };
>> +
>> +bool qnull_is_equal(const QObject *x, const QObject *y)
>> +{
>> +    return true;
>> +}
>> diff --git a/qobject/qobject.c b/qobject/qobject.c
>> index fe4fa10..af2869a 100644
>> --- a/qobject/qobject.c
>> +++ b/qobject/qobject.c
>> @@ -28,3 +28,33 @@ 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_QINT] = qint_is_equal,
>> +    [QTYPE_QSTRING] = qstring_is_equal,
>> +    [QTYPE_QDICT] = qdict_is_equal,
>> +    [QTYPE_QLIST] = qlist_is_equal,
>> +    [QTYPE_QFLOAT] = qfloat_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..a2b51fa 100644
>> --- a/qobject/qstring.c
>> +++ b/qobject/qstring.c
>> @@ -129,6 +129,15 @@ const char *qstring_get_str(const QString *qstring)
>>  }
>>  
>>  /**
>> + * qstring_is_equal(): Tests 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
>>   */
> 
> Address my nitpicks, and either provide a unit test or convince me it's
> not worth the trouble, and you may add

Unfortunately (for me), it's always worth the trouble.

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

Thanks again!

Max


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

Re: [Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-06-21 18:24, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> Reviewed-by: Kevin Wolf <kwolf@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            |  1 +
>>>  target/i386/cpu.c          |  6 +-----
>>>  tests/check-qnull.c        |  2 +-
>>>  6 files changed, 30 insertions(+), 14 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..69555ac
>>> --- /dev/null
>>> +++ b/include/qapi/qmp/qnull.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * QNull Module
>>> + *
>>> + * Copyright (C) 2009, 2017 Red Hat Inc.
>>> + *
>>> + * Authors:
>>> + *  Luiz Capitulino <lcapitulino@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.
>> 
>> Copy the boilerplate from qnull.c instead, factual correctness.
>
> Sorry, will do.

No need to be sorry!  Copying it from qobject.h along with the code was
the obvious thing to do.

[...]

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-06-21 18:06, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> Currently, bdrv_reopen_prepare() assumes that all BDS options are
>>> strings. However, this is not the case if the BDS has been created
>>> through the json: pseudo-protocol or blockdev-add.
>>>
>>> Note that the user-invokable reopen command is an HMP command, so you
>>> can only specify strings there. Therefore, specifying a non-string
>>> option with the "same" value as it was when originally created will now
>>> return an error because the values are supposedly similar (and there is
>>> no way for the user to circumvent this but to just not specify the
>>> option again -- however, this is still strictly better than just
>>> crashing).
>>>
>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  block.c | 15 +++------------
>>>  1 file changed, 3 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index fa1d06d..23424d5 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2950,19 +2950,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>>          const QDictEntry *entry = qdict_first(reopen_state->options);
>>>  
>>>          do {
>>> -            QString *new_obj = qobject_to_qstring(entry->value);
>>> -            const char *new = qstring_get_str(new_obj);
>>> -            /*
>>> -             * Caution: while qdict_get_try_str() is fine, getting
>>> -             * non-string types would require more care.  When
>>> -             * bs->options come from -blockdev or blockdev_add, its
>>> -             * members are typed according to the QAPI schema, but
>>> -             * when they come from -drive, they're all QString.
>>> -             */
>> 
>> Your commit message makes me suspect this comment still applies in some
>> form.  Does it?
>
> The only thing that I can think of that may be applicable is what I
> wrote in the commit message; this fails now:
>
> $ qemu-io -c 'reopen -o size=65536' \
>     "json:{'driver':'null-co','size':65536}"
> Cannot change the option 'size'
>
> As I wrote in the commit message, though, I don't think this is too bad.
> First, before, it just crashed, so this definitely is better behavior.
>
> Secondly, I think this is just good for convenience; it allows the user
> to specify an option (to "change" it) even if the block driver doesn't
> support changing it. If it actually has not change, this is supposed to
> be handled gracefully. But sometimes we cannot easily handle it, so...
> We just give an error.
>
> I suspect that at some point we want to fix this by having everything
> correctly typed internally...? Until then, in my opinion, we can just
> not provide this feature.

Correcting the types in a QDict to match the schema is no easier than
converting it to a QAPI generated C type.  In my opinion we should work
towards the latter (QAPIfy stuff) instead of the former (dig us ever
deeper into the QObject hole).

> However, I should have probably not just deleted the comment and hoped
> everyone can find the necessary information through git-blame. I should
> leave a comment about this here.

Yes, please!

> Or we do make an effort to provide this test-unchanged functionality for
> every case, in which case we would have to convert either string options
> to the correct type (according to the schema) here (but if that were so
> simple, we could do that centrally in bdrv_open() already, right?) or

The one way we already have to check QObject types against the schema is
the QObject input visitor.  So, to get a QObject with correct types, you
can convert to the QAPI-generated C type with the appropriate variant of
the QObject input visitor, then right back with the QObject output
visitor.

But once we have the QAPI-generated C type, we should simply use *that*
instead of dicking around with QDict.  In other words: QAPIfy.

"Appropriate variant" means you need to choose between the variant that
expects correct scalar types (appropriate when the QObject comes from
JSON) and the keyval variant, which expects strings (appropriate when
the QObject comes from keyval_parse()).

> convert typed options to strings and compare them -- but since there are
> probably multiple different strings that can mean the same value for
> whatever type the option has, this won't work in every case either.

Converting to string throws away information.  Not sure that works.

> So I'm for leaving a comment and leaving this not quite as it should be
> until we have fixed all of the block layer (O:-)). Maybe you have a
> better idea though, which would be great.

Let's go with a comment.

Re: [Qemu-devel] [PATCH v2 2/4] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-06-21 18:43, 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>
>>> ---
>>>  include/qapi/qmp/qbool.h   |  1 +
>>>  include/qapi/qmp/qdict.h   |  1 +
>>>  include/qapi/qmp/qfloat.h  |  1 +
>>>  include/qapi/qmp/qint.h    |  1 +
>>>  include/qapi/qmp/qlist.h   |  1 +
>>>  include/qapi/qmp/qnull.h   |  2 ++
>>>  include/qapi/qmp/qobject.h |  9 +++++++++
>>>  include/qapi/qmp/qstring.h |  1 +
>>>  qobject/qbool.c            |  8 ++++++++
>>>  qobject/qdict.c            | 28 ++++++++++++++++++++++++++++
>>>  qobject/qfloat.c           |  8 ++++++++
>>>  qobject/qint.c             |  8 ++++++++
>>>  qobject/qlist.c            | 30 ++++++++++++++++++++++++++++++
>>>  qobject/qnull.c            |  5 +++++
>>>  qobject/qobject.c          | 30 ++++++++++++++++++++++++++++++
>>>  qobject/qstring.c          |  9 +++++++++
>>>  16 files changed, 143 insertions(+)
>> 
>> No unit test?
>
> *cough*
>
>>> 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 188440a..134a526 100644
>>> --- a/include/qapi/qmp/qdict.h
>>> +++ b/include/qapi/qmp/qdict.h
>>> @@ -41,6 +41,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/qfloat.h b/include/qapi/qmp/qfloat.h
>>> index b5d1583..318e91e 100644
>>> --- a/include/qapi/qmp/qfloat.h
>>> +++ b/include/qapi/qmp/qfloat.h
>>> @@ -24,6 +24,7 @@ typedef struct QFloat {
>>>  QFloat *qfloat_from_double(double value);
>>>  double qfloat_get_double(const QFloat *qi);
>>>  QFloat *qobject_to_qfloat(const QObject *obj);
>>> +bool qfloat_is_equal(const QObject *x, const QObject *y);
>>>  void qfloat_destroy_obj(QObject *obj);
>>>  
>>>  #endif /* QFLOAT_H */
>>> diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
>>> index 3aaff76..20975da 100644
>>> --- a/include/qapi/qmp/qint.h
>>> +++ b/include/qapi/qmp/qint.h
>>> @@ -23,6 +23,7 @@ typedef struct QInt {
>>>  QInt *qint_from_int(int64_t value);
>>>  int64_t qint_get_int(const QInt *qi);
>>>  QInt *qobject_to_qint(const QObject *obj);
>>> +bool qint_is_equal(const QObject *x, const QObject *y);
>>>  void qint_destroy_obj(QObject *obj);
>>>  
>>>  #endif /* QINT_H */
>>> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
>>> index 5dc4ed9..4380a5b 100644
>>> --- a/include/qapi/qmp/qlist.h
>>> +++ b/include/qapi/qmp/qlist.h
>>> @@ -57,6 +57,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 69555ac..9299683 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/qobject.h b/include/qapi/qmp/qobject.h
>>> index ef1d1a9..cac72e3 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(): Returns whether the two objects are equal.
>> 
>> Imperative mood, please: Return whether...
>
> OK, will do here and everywhere else.
>
>>> + *
>>> + * Any of the pointers may be NULL; will return true if both are. Will always
>>> + * return false if only one is (therefore a QNull object is not considered equal
>>> + * to a NULL pointer).
>>> + */
>> 
>> Humor me: wrap comment lines around column 70, and put two spaces
>> between sentences.
>
> Do I hear "one checkpatch.pl per subsystem"?

Nah, you hear "would you mind doing me a favor if I ask nicely?"

> Yes, I know, there's no point to. Why should anyone not break comments
> after 70 lines and not use two spaces after full stops? O:-)

That too ;)

>> Same for the other function comments.
>
> Sure, will do.
>
>>> +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..f1d1719 100644
>>> --- a/qobject/qbool.c
>>> +++ b/qobject/qbool.c
>>> @@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
>>>  }
>>>  
>>>  /**
>>> + * qbool_is_equal(): Tests whether the two QBools are equal
>> 
>> Return whether ...
>> 
>>> + */
>>> +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 88e2ecd..ca919bc 100644
>>> --- a/qobject/qdict.c
>>> +++ b/qobject/qdict.c
>>> @@ -410,6 +410,34 @@ void qdict_del(QDict *qdict, const char *key)
>>>  }
>>>  
>>>  /**
>>> + * qdict_is_equal(): Tests 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), *dict_y = qobject_to_qdict(y);
>> 
>> I'm fine with multiple initializers in one declaration, but I think this
>> one would look tidier as
>> 
>>        const QDict *dict_x = qobject_to_qdict(x);
>>        const QDict *dict_y = qobject_to_qdict(y);
>
> I don't mind.
>
>>> +    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/qfloat.c b/qobject/qfloat.c
>>> index d5da847..291ecc4 100644
>>> --- a/qobject/qfloat.c
>>> +++ b/qobject/qfloat.c
>>> @@ -52,6 +52,14 @@ QFloat *qobject_to_qfloat(const QObject *obj)
>>>  }
>>>  
>>>  /**
>>> + * qfloat_is_equal(): Tests whether the two QFloats are equal
>>> + */
>>> +bool qfloat_is_equal(const QObject *x, const QObject *y)
>>> +{
>>> +    return qobject_to_qfloat(x)->value == qobject_to_qfloat(y)->value;
>>> +}
>>> +
>>> +/**
>>>   * qfloat_destroy_obj(): Free all memory allocated by a
>>>   * QFloat object
>>>   */
>>> diff --git a/qobject/qint.c b/qobject/qint.c
>>> index d7d1b30..f638cb7 100644
>>> --- a/qobject/qint.c
>>> +++ b/qobject/qint.c
>>> @@ -51,6 +51,14 @@ QInt *qobject_to_qint(const QObject *obj)
>>>  }
>>>  
>>>  /**
>>> + * qint_is_equal(): Tests whether the two QInts are equal
>>> + */
>>> +bool qint_is_equal(const QObject *x, const QObject *y)
>>> +{
>>> +    return qobject_to_qint(x)->value == qobject_to_qint(y)->value;
>>> +}
>>> +
>>> +/**
>>>   * qint_destroy_obj(): Free all memory allocated by a
>>>   * QInt object
>>>   */
>>> diff --git a/qobject/qlist.c b/qobject/qlist.c
>>> index 86b60cb..652fc53 100644
>>> --- a/qobject/qlist.c
>>> +++ b/qobject/qlist.c
>>> @@ -140,6 +140,36 @@ QList *qobject_to_qlist(const QObject *obj)
>>>  }
>>>  
>>>  /**
>>> + * qlist_is_equal(): Tests 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), *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 b3cc85e..af5453d 100644
>>> --- a/qobject/qnull.c
>>> +++ b/qobject/qnull.c
>>> @@ -19,3 +19,8 @@ QObject qnull_ = {
>>>      .type = QTYPE_QNULL,
>>>      .refcnt = 1,
>>>  };
>>> +
>>> +bool qnull_is_equal(const QObject *x, const QObject *y)
>>> +{
>>> +    return true;
>>> +}
>>> diff --git a/qobject/qobject.c b/qobject/qobject.c
>>> index fe4fa10..af2869a 100644
>>> --- a/qobject/qobject.c
>>> +++ b/qobject/qobject.c
>>> @@ -28,3 +28,33 @@ 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_QINT] = qint_is_equal,
>>> +    [QTYPE_QSTRING] = qstring_is_equal,
>>> +    [QTYPE_QDICT] = qdict_is_equal,
>>> +    [QTYPE_QLIST] = qlist_is_equal,
>>> +    [QTYPE_QFLOAT] = qfloat_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..a2b51fa 100644
>>> --- a/qobject/qstring.c
>>> +++ b/qobject/qstring.c
>>> @@ -129,6 +129,15 @@ const char *qstring_get_str(const QString *qstring)
>>>  }
>>>  
>>>  /**
>>> + * qstring_is_equal(): Tests 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
>>>   */
>> 
>> Address my nitpicks, and either provide a unit test or convince me it's
>> not worth the trouble, and you may add
>
> Unfortunately (for me), it's always worth the trouble.

Awesome!

>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks again!
>
> Max

Re: [Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Max Reitz]

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

On 2017-06-22 16:41, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2017-06-21 18:24, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> Reviewed-by: Kevin Wolf <kwolf@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            |  1 +
>>>>  target/i386/cpu.c          |  6 +-----
>>>>  tests/check-qnull.c        |  2 +-
>>>>  6 files changed, 30 insertions(+), 14 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..69555ac
>>>> --- /dev/null
>>>> +++ b/include/qapi/qmp/qnull.h
>>>> @@ -0,0 +1,26 @@
>>>> +/*
>>>> + * QNull Module
>>>> + *
>>>> + * Copyright (C) 2009, 2017 Red Hat Inc.
>>>> + *
>>>> + * Authors:
>>>> + *  Luiz Capitulino <lcapitulino@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.
>>>
>>> Copy the boilerplate from qnull.c instead, factual correctness.
>>
>> Sorry, will do.
> 
> No need to be sorry!

I was when I noticed that it was your authorship I failed to announce. :-)

Max


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