Re: [Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

2017-08-15 Thread Eduardo Habkost 
On Tue, Aug 15, 2017 at 11:16:57AM -0500, Eric Blake wrote:
> On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> > The helper function will be useful when writing support code to
> > deal with device slot information.
> > 
> > TODO: documentation is incomplete and unclear, needs to be
> > improved.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  include/qapi/util.h| 39 +
> >  qapi/qapi-util.c   | 66 
> > ++
> >  tests/test-qapi-util.c | 53 
> >  3 files changed, 158 insertions(+)
> > 
> 
> > +/**
> > + * qobject_compare:
> > + *
> > + * Compare the value of @a and @b.
> > + *
> > + * If @a and @b have the same type and the same value (see list
> > + * of supported types below), return 0.
> > + *
> > + * If @a and @b are both strings, return strcmp(a, b).
> > + *
> > + * If @a and @b are numbers, return a negative value if a < b,
> > + * and a positive value if a > b.
> > + *
> > + * Otherwise (if @a and @b are not the same, have different types,
> > + * are of an unsupported type, or are different), return a non-zero value.
> 
> Is this number going to be commutative and distributive, in order to
> provide stable qsort()ing?  That is, if comparing a and b gives a
> positive number, then comparing b and a should give a negative number;
> and if comparing a and b then b and c results in two positive numbers,
> then comparing a and c should also give a positive number.  It is
> unclear from the documentation whether you are able to make this
> guarantee; and without it, it is unsafe to use this comparator in places
> that require stability.

No, I don't make that guarantee when the types don't match or in
the case of unsupported types.  That's a limitation of this
implementation.

Guaranteeing it when types don't match should be easy.
Guaranteeing it in the case of QTYPE_QDICT looks a bit harder.
Probably it's easier to simply implement full dictionary
comparison.

> 
> > + *
> > + * Note that this function doesn't support some types, and may
> > + * return false if the types are unsupported, or if the types don't
> > + * match exactly.
> 
> How is a return of false (== 0, which also means equivalent) correct?

This is a documentation bug.  Leftover from a version that
returned a boolean value (true for equal, false for not equal) in
the past.

> 
> > + *
> > + * Supported types:
> > + * - QTYPE_QNULL
> > + * - QTYPE_QSTRING
> > + * - QTYPE_QBOOL
> > + * - QTYPE_QNUM (integers only)
> > + * - QTYPE_QLIST
> > + *
> > + * Unsupported (always return false):
> > + * - QTYPE_QNUM (non-integer values)
> > + * - QTYPE_QDICT
> > + *
> > + * TODO: rewrite documentation to be clearer.
> > + * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.
> 
> There's another patch series pending for qobject_is_equal(); should
> these two patches share approaches or even code?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01134.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02459.html

I will take a look.  Thanks!

-- 
Eduardo

Re: [Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

2017-08-15 Thread Eric Blake 
On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> The helper function will be useful when writing support code to
> deal with device slot information.
> 
> TODO: documentation is incomplete and unclear, needs to be
> improved.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  include/qapi/util.h| 39 +
>  qapi/qapi-util.c   | 66 
> ++
>  tests/test-qapi-util.c | 53 
>  3 files changed, 158 insertions(+)
> 

> +/**
> + * qobject_compare:
> + *
> + * Compare the value of @a and @b.
> + *
> + * If @a and @b have the same type and the same value (see list
> + * of supported types below), return 0.
> + *
> + * If @a and @b are both strings, return strcmp(a, b).
> + *
> + * If @a and @b are numbers, return a negative value if a < b,
> + * and a positive value if a > b.
> + *
> + * Otherwise (if @a and @b are not the same, have different types,
> + * are of an unsupported type, or are different), return a non-zero value.

Is this number going to be commutative and distributive, in order to
provide stable qsort()ing?  That is, if comparing a and b gives a
positive number, then comparing b and a should give a negative number;
and if comparing a and b then b and c results in two positive numbers,
then comparing a and c should also give a positive number.  It is
unclear from the documentation whether you are able to make this
guarantee; and without it, it is unsafe to use this comparator in places
that require stability.

> + *
> + * Note that this function doesn't support some types, and may
> + * return false if the types are unsupported, or if the types don't
> + * match exactly.

How is a return of false (== 0, which also means equivalent) correct?

> + *
> + * Supported types:
> + * - QTYPE_QNULL
> + * - QTYPE_QSTRING
> + * - QTYPE_QBOOL
> + * - QTYPE_QNUM (integers only)
> + * - QTYPE_QLIST
> + *
> + * Unsupported (always return false):
> + * - QTYPE_QNUM (non-integer values)
> + * - QTYPE_QDICT
> + *
> + * TODO: rewrite documentation to be clearer.
> + * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.

There's another patch series pending for qobject_is_equal(); should
these two patches share approaches or even code?

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01134.html
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02459.html

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

2017-08-14 Thread Eduardo Habkost 
The helper function will be useful when writing support code to
deal with device slot information.

TODO: documentation is incomplete and unclear, needs to be
improved.

Signed-off-by: Eduardo Habkost 
---
 include/qapi/util.h| 39 +
 qapi/qapi-util.c   | 66 ++
 tests/test-qapi-util.c | 53 
 3 files changed, 158 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7436ed8..d4562c4 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,9 +11,48 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
+#include "qapi/qmp/qobject.h"
+#include "qapi-types.h"
+
 int qapi_enum_parse(const char * const lookup[], const char *buf,
 int max, int def, Error **errp);
 
 int parse_qapi_name(const char *name, bool complete);
 
+/**
+ * qobject_compare:
+ *
+ * Compare the value of @a and @b.
+ *
+ * If @a and @b have the same type and the same value (see list
+ * of supported types below), return 0.
+ *
+ * If @a and @b are both strings, return strcmp(a, b).
+ *
+ * If @a and @b are numbers, return a negative value if a < b,
+ * and a positive value if a > b.
+ *
+ * Otherwise (if @a and @b are not the same, have different types,
+ * are of an unsupported type, or are different), return a non-zero value.
+ *
+ * Note that this function doesn't support some types, and may
+ * return false if the types are unsupported, or if the types don't
+ * match exactly.
+ *
+ * Supported types:
+ * - QTYPE_QNULL
+ * - QTYPE_QSTRING
+ * - QTYPE_QBOOL
+ * - QTYPE_QNUM (integers only)
+ * - QTYPE_QLIST
+ *
+ * Unsupported (always return false):
+ * - QTYPE_QNUM (non-integer values)
+ * - QTYPE_QDICT
+ *
+ * TODO: rewrite documentation to be clearer.
+ * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.
+ */
+int qobject_compare(QObject *a, QObject *b);
+
 #endif
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 46eda7d..67c5e82 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -13,6 +13,9 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/util.h"
 
 int qapi_enum_parse(const char * const lookup[], const char *buf,
@@ -80,3 +83,66 @@ int parse_qapi_name(const char *str, bool complete)
 }
 return p - str;
 }
+
+static int qnum_compare(QNum *a, QNum *b)
+{
+int64_t ia, ib;
+bool va = qnum_get_try_int(a, );
+bool vb = qnum_get_try_int(b, );
+
+if (va && vb) {
+return (ia < ib) ? -1 : (ia > ib) ? 1 : 0;
+}
+
+/*TODO: uint, double */
+return -1;
+}
+
+static int qlist_compare(QList *a, QList *b)
+{
+const QListEntry *ea, *eb;
+
+for (ea = qlist_first(a), eb = qlist_first(b);
+ ea && eb;
+ ea = qlist_next(ea), eb = qlist_next(eb)) {
+QObject *va = qlist_entry_obj(ea);
+QObject *vb = qlist_entry_obj(eb);
+int c = qobject_compare(va, vb);
+if (c) {
+return c;
+}
+}
+
+if (eb) {
+return -1;
+} else if (ea) {
+return 1;
+} else {
+return 0;
+}
+}
+
+int qobject_compare(QObject *a, QObject *b)
+{
+QType ta = qobject_type(a);
+QType tb = qobject_type(b);
+
+if (ta != tb) {
+return -1;
+}
+
+switch (ta) {
+case QTYPE_QNULL:
+return true;
+case QTYPE_QNUM:
+return qnum_compare(qobject_to_qnum(a), qobject_to_qnum(b));
+case QTYPE_QSTRING:
+return strcmp(qstring_get_str(qobject_to_qstring(a)), 
qstring_get_str(qobject_to_qstring(b)));
+case QTYPE_QBOOL:
+return (int)qbool_get_bool(qobject_to_qbool(a)) - 
(int)qbool_get_bool(qobject_to_qbool(b));
+case QTYPE_QLIST:
+return qlist_compare(qobject_to_qlist(a), qobject_to_qlist(b));
+default:
+return -1;
+}
+}
diff --git a/tests/test-qapi-util.c b/tests/test-qapi-util.c
index e869757..c0cf46c 100644
--- a/tests/test-qapi-util.c
+++ b/tests/test-qapi-util.c
@@ -13,6 +13,10 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/util.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qjson.h"
 #include "test-qapi-types.h"
 
 static void test_qapi_enum_parse(void)
@@ -75,11 +79,60 @@ static void test_parse_qapi_name(void)
 g_assert(ret == -1);
 }
 
+static void test_qobject_compare(void)
+{
+QString *a1 = qstring_from_str("abc");
+QString *a2 = qstring_from_str("abc");
+QString *b = qstring_from_str("bcd");
+QNum *i1 = qnum_from_int(100);
+QNum *i2 = qnum_from_int(100);
+QNum *j = qnum_from_int(200);
+QList *l1 = qlist_new();
+QList *l2 = qlist_new();
+QList *m = qlist_new();
+
+qlist_append_int(l1, 100);
+qlist_append_int(l1, 200);
+qlist_append_int(l2, 100);
+qlist_append_int(l2,