[Qemu-devel] [PATCH] qapi: Accept 'null' in QMP

* [Qemu-devel] [PATCH] qapi: Accept 'null' in QMP
@ 2015-04-02 19:31 Eric Blake
  2015-04-07 12:46 ` Alberto Garcia
  2015-04-30  3:27 ` Eric Blake
  0 siblings, 2 replies; 4+ messages in thread
From: Eric Blake @ 2015-04-02 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, akong, berto, mdroth, armbru

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.

The existing QDict, QList, and QString all represent something
that is a pointer in C and could therefore reasonably be
associated with NULL; on the other hand, I'm not sure how much
of the code base might break if these three types changed to
allow a null value, since right now, all three are always
non-null once created.  Besides, picking just one of the three
types for handling the JSON 'null' literal feels arbitrary.
So instead, this patch creates a new QObject subtype: QNull.

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
{ } by omitting the key, in order 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 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 that can come at a later day.

In addition to the 'make check-unit' improvement, 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>

---

Posting this as an alternative (or as additional ideas on top of)
Markus' patch here:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00350.html

(I had written it as part of my larger work on dropping qapi nested
types, but looks like I can post it now without holding up that series)

---
 include/qapi/qmp/qnull.h   | 23 +++++++++++++++++++
 include/qapi/qmp/qobject.h |  3 ++-
 qobject/Makefile.objs      |  2 +-
 qobject/json-parser.c      |  3 +++
 qobject/qjson.c            |  4 ++++
 qobject/qnull.c            | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/check-qjson.c        | 15 +++++++++++--
 7 files changed, 102 insertions(+), 4 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 qobject/qnull.c

diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
new file mode 100644
index 0000000..d2992ca
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,23 @@
+/*
+ * QBool Module
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QNULL_H
+#define QNULL_H
+
+#include "qapi/qmp/qobject.h"
+
+typedef struct QNull {
+    QObject_HEAD;
+} QNull;
+
+QNull *qnull_new(void);
+QNull *qobject_to_qnull(const QObject *obj);
+
+#endif /* QNULL_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index d0bbc7c..48580bd 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>
@@ -43,6 +43,7 @@ typedef enum {
     QTYPE_QLIST,
     QTYPE_QFLOAT,
     QTYPE_QBOOL,
+    QTYPE_QNULL,
     QTYPE_QERROR,
     QTYPE_MAX,
 } qtype_code;
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index c9ff59c..10a60e3 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 = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qnull.o
 util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 util-obj-y += qerror.o
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 4288267..342adc6 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -20,6 +20,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qfloat.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/qerror.h"
@@ -561,6 +562,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 = QOBJECT(qnull_new());
     } else {
         parse_error(ctxt, token, "invalid keyword `%s'", token_get_value(token));
         goto out;
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 12c576d..6350da5 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qfloat.h"
 #include "qapi/qmp/qdict.h"

@@ -258,6 +259,9 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
         }
         break;
     }
+    case QTYPE_QNULL:
+        qstring_append(str, "null");
+        break;
     case QTYPE_QERROR:
         /* XXX: should QError be emitted? */
     case QTYPE_NONE:
diff --git a/qobject/qnull.c b/qobject/qnull.c
new file mode 100644
index 0000000..9f7c360
--- /dev/null
+++ b/qobject/qnull.c
@@ -0,0 +1,56 @@
+/*
+ * QNull Module
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qapi/qmp/qnull.h"
+#include "qapi/qmp/qobject.h"
+#include "qemu-common.h"
+
+static void qnull_destroy_obj(QObject *obj);
+
+static const QType qnull_type = {
+    .code = QTYPE_QNULL,
+    .destroy = qnull_destroy_obj,
+};
+
+/**
+ * qnull_new(): Create a new QNull
+ *
+ * Return strong reference.
+ */
+QNull *qnull_new(void)
+{
+    QNull *qn;
+
+    qn = g_malloc(sizeof(*qn));
+    QOBJECT_INIT(qn, &qnull_type);
+
+    return qn;
+}
+
+/**
+ * qobject_to_qnull(): Convert a QObject into a QNull
+ */
+QNull *qobject_to_qnull(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QNULL)
+        return NULL;
+
+    return container_of(obj, QNull, base);
+}
+
+/**
+ * qnull_destroy_obj(): Free all memory allocated by a
+ * QNull object
+ */
+static void qnull_destroy_obj(QObject *obj)
+{
+    assert(obj != NULL);
+    g_free(qobject_to_qnull(obj));
+}
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 95497a0..641c1f8 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>
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qfloat.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qjson.h"

 #include "qemu-common.h"
@@ -1005,6 +1006,7 @@ static void keyword_literal(void)
 {
     QObject *obj;
     QBool *qbool;
+    QNull *qnull;
     QString *str;

     obj = qobject_from_json("true");
@@ -1041,7 +1043,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 +1052,15 @@ 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);
+
+    qnull = qobject_to_qnull(obj);
+    g_assert(qnull);
+
+    QDECREF(qnull);
 }

 typedef struct LiteralQDictEntry LiteralQDictEntry;
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread