[Qemu-devel] [PATCH v2 0/3] parse 'null' literal in QMP

[Qemu-devel] [PATCH v2 0/3] parse 'null' literal in QMP

[Eric Blake]

Here's my attempt to merge the best points of Markus' approach [1]
(patches 16-18 of that series - benefit of smaller patches and fewer
malloc calls) and my approach [2] (benefit of a testsuite addition
and more detailed commit messages), while fixing the typos that both
of us had in v1.

[1]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00342.html
[2]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00623.html

Eric Blake (1):
  json-parser: Accept 'null' in QMP

Markus Armbruster (2):
  qobject: Clean up around qtype_code
  qobject: Add a special null QObject

 block/qapi.c               |  3 ---
 include/hw/qdev-core.h     |  2 +-
 include/qapi/qmp/qobject.h | 13 +++++++++++--
 qobject/Makefile.objs      |  2 +-
 qobject/json-parser.c      |  2 ++
 qobject/qjson.c            |  6 ++++--
 qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
 tests/check-qjson.c        | 15 +++++++++++++--
 8 files changed, 61 insertions(+), 11 deletions(-)
 create mode 100644 qobject/qnull.c

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/3] qobject: Clean up around qtype_code

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

QTYPE_NONE is a sentinel value.  No QObject has this type code.
Document it properly.

Fix dump_qobject() to abort() on QTYPE_NONE, just like for any other
invalid type code.

Fix to_json() to abort() on all invalid type codes, not just
QTYPE_MAX.

Clean up Property member qtype's type: it's a qtype_code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qapi.c               | 3 ---
 include/hw/qdev-core.h     | 2 +-
 include/qapi/qmp/qobject.h | 2 +-
 qobject/qjson.c            | 3 +--
 4 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 063dd1b..18d2b95 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -523,9 +523,6 @@ static void dump_qobject(fprintf_function func_fprintf, void *f,
             QDECREF(value);
             break;
         }
-        case QTYPE_NONE:
-            break;
-        case QTYPE_MAX:
         default:
             abort();
     }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..9a0ee30 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -226,7 +226,7 @@ struct Property {
     PropertyInfo *info;
     int          offset;
     uint8_t      bitnr;
-    uint8_t      qtype;
+    qtype_code   qtype;
     int64_t      defval;
     int          arrayoffset;
     PropertyInfo *arrayinfo;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index d0bbc7c..0991296 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -36,7 +36,7 @@
 #include <assert.h>

 typedef enum {
-    QTYPE_NONE,
+    QTYPE_NONE,    /* sentinel value, no QObject has this type code */
     QTYPE_QINT,
     QTYPE_QSTRING,
     QTYPE_QDICT,
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 12c576d..f2857c1 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -260,9 +260,8 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
     case QTYPE_QERROR:
         /* XXX: should QError be emitted? */
-    case QTYPE_NONE:
         break;
-    case QTYPE_MAX:
+    default:
         abort();
     }
 }
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/3] qobject: Add a special null QObject

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

I'm going to fix the JSON parser to recognize null.  The obvious
representation of JSON null as (QObject *)NULL doesn't work, because
the parser already uses it as an error value.  Perhaps we should
change it to free NULL for null, but that's more than I can do right
now.  Create a special null QObject instead.

The existing QDict, QList, and QString all represent something that
is a pointer in C and could therefore be associated with NULL.  But
right now, all three of these sub-types are always non-null once
created, so the new null sentinel object is intentionally unrelated
to them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qmp/qobject.h | 11 ++++++++++-
 qobject/Makefile.objs      |  2 +-
 qobject/qjson.c            |  3 +++
 qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100644 qobject/qnull.c

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 0991296..84b2d9f 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -3,7 +3,7 @@
  *
  * Based on ideas by Avi Kivity <avi@redhat.com>
  *
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009, 2015 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino <lcapitulino@redhat.com>
@@ -37,6 +37,7 @@

 typedef enum {
     QTYPE_NONE,    /* sentinel value, no QObject has this type code */
+    QTYPE_QNULL,
     QTYPE_QINT,
     QTYPE_QSTRING,
     QTYPE_QDICT,
@@ -110,4 +111,12 @@ static inline qtype_code qobject_type(const QObject *obj)
     return obj->type->code;
 }

+extern QObject qnull_;
+
+static inline QObject *qnull(void)
+{
+    qobject_incref(&qnull_);
+    return &qnull_;
+}
+
 #endif /* QOBJECT_H */
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index c9ff59c..f7595f5 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,3 +1,3 @@
-util-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
+util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
 util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 util-obj-y += qerror.o
diff --git a/qobject/qjson.c b/qobject/qjson.c
index f2857c1..846733d 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -127,6 +127,9 @@ static void to_json_list_iter(QObject *obj, void *opaque)
 static void to_json(const QObject *obj, QString *str, int pretty, int indent)
 {
     switch (qobject_type(obj)) {
+    case QTYPE_QNULL:
+        qstring_append(str, "null");
+        break;
     case QTYPE_QINT: {
         QInt *val = qobject_to_qint(obj);
         char buffer[1024];
diff --git a/qobject/qnull.c b/qobject/qnull.c
new file mode 100644
index 0000000..9873e26
--- /dev/null
+++ b/qobject/qnull.c
@@ -0,0 +1,29 @@
+/*
+ * 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.
+ */
+
+#include "qemu-common.h"
+#include "qapi/qmp/qobject.h"
+
+static void qnull_destroy_obj(QObject *obj)
+{
+    assert(0);
+}
+
+static const QType qnull_type = {
+    .code = QTYPE_QNULL,
+    .destroy = qnull_destroy_obj,
+};
+
+QObject qnull_ = {
+    .type = &qnull_type,
+    .refcnt = 1,
+};
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/3] json-parser: Accept 'null' in QMP

[Eric Blake]

We document that in QMP, the client may send any json-value
for the optional "id" key, and then return that same value
on reply (both success and failures, insofar as the failure
happened after parsing the id).  [Note that the output may
not be identical to the input, as whitespace may change and
since we may reorder keys within a json-object, but that this
still constitutes the same json-value].  However, we were not
handling the JSON literal null, which counts as a json-value
per RFC 7159.

Also, down the road, given the QAPI schema of {'*foo':'str'} or
{'*foo':'ComplexType'}, we could decide to allow the QMP client
to pass { "foo":null } instead of the current representation of
{ } where omitting the key is the only way to get at the default
NULL value.  Such a change might be useful for argument
introspection (if a type in older qemu lacks 'foo' altogether,
then an explicit "foo":null probe will force an easily
distinguished error message for whether the optional "foo" key
is even understood in newer qemu).  And if we add default values
to optional arguments, allowing an explicit null would be
required for getting a NULL value associated with an optional
string that has a non-null default.  But all that can come at a
later day.

The 'check-unit' testsuite is enhanced to test that parsing
produces the same object as explicitly requesting a reference
to the special qnull object.  In addition, I tested with:

$ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
{"execute":"qmp_capabilities","id":null}
{"return": {}, "id": null}
{"id":{"a":null,"b":[1,null]},"execute":"quit"}
{"return": {}, "id": {"a": null, "b": [1, null]}}
{"timestamp": {"seconds": 1427742379, "microseconds": 423128}, "event": "SHUTDOWN"}

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/json-parser.c |  2 ++
 tests/check-qjson.c   | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 4288267..717cb8f 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -561,6 +561,8 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
         ret = QOBJECT(qbool_from_int(true));
     } else if (token_is_keyword(token, "false")) {
         ret = QOBJECT(qbool_from_int(false));
+    } else if (token_is_keyword(token, "null")) {
+        ret = qnull();
     } else {
         parse_error(ctxt, token, "invalid keyword `%s'", token_get_value(token));
         goto out;
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 95497a0..60e5b22 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1,6 +1,6 @@
 /*
  * Copyright IBM, Corp. 2009
- * Copyright (c) 2013 Red Hat Inc.
+ * Copyright (c) 2013, 2015 Red Hat Inc.
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
@@ -1005,6 +1005,7 @@ static void keyword_literal(void)
 {
     QObject *obj;
     QBool *qbool;
+    QObject *null;
     QString *str;

     obj = qobject_from_json("true");
@@ -1041,7 +1042,7 @@ static void keyword_literal(void)
     g_assert(qbool_get_int(qbool) == 0);

     QDECREF(qbool);
-    
+
     obj = qobject_from_jsonf("%i", true);
     g_assert(obj != NULL);
     g_assert(qobject_type(obj) == QTYPE_QBOOL);
@@ -1050,6 +1051,16 @@ static void keyword_literal(void)
     g_assert(qbool_get_int(qbool) != 0);

     QDECREF(qbool);
+
+    obj = qobject_from_json("null");
+    g_assert(obj != NULL);
+    g_assert(qobject_type(obj) == QTYPE_QNULL);
+
+    null = qnull();
+    g_assert(null == obj);
+
+    qobject_decref(obj);
+    qobject_decref(null);
 }

 typedef struct LiteralQDictEntry LiteralQDictEntry;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 2/3] qobject: Add a special null QObject

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> From: Markus Armbruster <armbru@redhat.com>
>
> I'm going to fix the JSON parser to recognize null.  The obvious
> representation of JSON null as (QObject *)NULL doesn't work, because
> the parser already uses it as an error value.  Perhaps we should
> change it to free NULL for null, but that's more than I can do right
> now.  Create a special null QObject instead.

Keeping my S-o-B here would clarify commit message authorship.

> The existing QDict, QList, and QString all represent something that
> is a pointer in C and could therefore be associated with NULL.  But
> right now, all three of these sub-types are always non-null once
> created, so the new null sentinel object is intentionally unrelated
> to them.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

You fixed my pasto and updated a copyright note.  Thanks.

Re: [Qemu-devel] [PATCH v2 0/3] parse 'null' literal in QMP

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Here's my attempt to merge the best points of Markus' approach [1]
> (patches 16-18 of that series - benefit of smaller patches and fewer
> malloc calls) and my approach [2] (benefit of a testsuite addition
> and more detailed commit messages), while fixing the typos that both
> of us had in v1.
>
> [1]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00342.html
> [2]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00623.html

Looks ready.  Luiz?

Re: [Qemu-devel] [PATCH v2 0/3] parse 'null' literal in QMP

[Luiz Capitulino]

On Thu, 30 Apr 2015 14:21:02 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eric Blake <eblake@redhat.com> writes:
> 
> > Here's my attempt to merge the best points of Markus' approach [1]
> > (patches 16-18 of that series - benefit of smaller patches and fewer
> > malloc calls) and my approach [2] (benefit of a testsuite addition
> > and more detailed commit messages), while fixing the typos that both
> > of us had in v1.
> >
> > [1]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00342.html
> > [2]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00623.html
> 
> Looks ready.  Luiz?

It's in my queue.

Re: [Qemu-devel] [PATCH v2 0/3] parse 'null' literal in QMP

[Luiz Capitulino]

On Wed, 29 Apr 2015 15:35:03 -0600
Eric Blake <eblake@redhat.com> wrote:

> Here's my attempt to merge the best points of Markus' approach [1]
> (patches 16-18 of that series - benefit of smaller patches and fewer
> malloc calls) and my approach [2] (benefit of a testsuite addition
> and more detailed commit messages), while fixing the typos that both
> of us had in v1.
> 
> [1]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00342.html
> [2]https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00623.html

Applied to the qmp branch, thanks.

> 
> Eric Blake (1):
>   json-parser: Accept 'null' in QMP
> 
> Markus Armbruster (2):
>   qobject: Clean up around qtype_code
>   qobject: Add a special null QObject
> 
>  block/qapi.c               |  3 ---
>  include/hw/qdev-core.h     |  2 +-
>  include/qapi/qmp/qobject.h | 13 +++++++++++--
>  qobject/Makefile.objs      |  2 +-
>  qobject/json-parser.c      |  2 ++
>  qobject/qjson.c            |  6 ++++--
>  qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
>  tests/check-qjson.c        | 15 +++++++++++++--
>  8 files changed, 61 insertions(+), 11 deletions(-)
>  create mode 100644 qobject/qnull.c
>