[Qemu-devel] [PATCH v4 29/28] qapi: Add strict mode to JSON output visitor

[Eric Blake]

Let the caller decide whether output must be strict JSON (and
raise an error on an attempt to output an encoding error or
non-finite number), vs. the status quo of relaxed (encoding
errors are rewritten to use substitute U+fffd characters,
and non-finite numbers are output).

Adjust the testsuite to cover this: check-qobject-json checks
relaxed mode (since qobject_to_json() is unchanged in behavior),
test-qmp-output-visitor checks that QObject doesn't care about
JSON restrictions, and test-json-output-visitor is now in
strict mode and flags the errors.

Signed-off-by: Eric Blake <address@hidden>

---
v4: split out as new patch
[was parts of 04/18 and 07/18 in v3]
---
 include/qapi/json-output-visitor.h |  7 ++++-
 include/qapi/qmp/qobject-json.h    |  3 +-
 qapi/json-output-visitor.c         | 19 ++++++++----
 qemu-img.c                         |  6 ++--
 qobject/qobject-json.c             | 61 ++++++++++++++++++++++++--------------
 tests/check-qobject-json.c         | 15 ++++++++--
 tests/test-json-output-visitor.c   | 17 +++++++++--
 tests/test-qmp-output-visitor.c    | 17 +++++++++++
 8 files changed, 107 insertions(+), 38 deletions(-)

diff --git a/include/qapi/json-output-visitor.h 
b/include/qapi/json-output-visitor.h
index 414f91b..2ce2758 100644
--- a/include/qapi/json-output-visitor.h
+++ b/include/qapi/json-output-visitor.h
@@ -23,7 +23,12 @@ typedef struct JsonOutputVisitor JsonOutputVisitor;
  *
  * If @pretty, make the output legible with newlines and indentation;
  * otherwise the output uses a single line.
+ *
+ * If @strict, attempts to output invalid JSON (strings not properly
+ * encoded in UTF-8, or Infinity or NaN as a number) will be treated
+ * as an error; otherwise, output is best-effort even if not pure
+ * JSON.
  */
-Visitor *json_output_visitor_new(bool pretty, char **result);
+Visitor *json_output_visitor_new(bool pretty, bool strict, char **result);

 #endif
diff --git a/include/qapi/qmp/qobject-json.h b/include/qapi/qmp/qobject-json.h
index 28e44b2..30bc753 100644
--- a/include/qapi/qmp/qobject-json.h
+++ b/include/qapi/qmp/qobject-json.h
@@ -31,7 +31,8 @@ QString *qobject_to_json(const QObject *obj, bool pretty);
  * (ignored for a top-level visit, must be set if part of a
  * dictionary, should be NULL if part of a list).
  */
-void qobject_visit_output(Visitor *v, const char *name, const QObject *obj);
+void qobject_visit_output(Visitor *v, const char *name, const QObject *obj,
+                          Error **errp);

 int qstring_append_json_string(QString *qstring, const char *str);
 int qstring_append_json_number(QString *qstring, double number);
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
index ba647d3..28e95ee 100644
--- a/qapi/json-output-visitor.c
+++ b/qapi/json-output-visitor.c
@@ -13,11 +13,13 @@
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qobject-json.h"
+#include "qapi/error.h"

 struct JsonOutputVisitor {
     Visitor visitor;
     QString *str;
     bool pretty;
+    bool strict;
     bool comma;
     unsigned int depth;
     char **result;
@@ -133,8 +135,10 @@ static void json_output_type_str(Visitor *v, const char 
*name, char **obj,

     assert(*obj);
     json_output_name(jov, name);
-    /* FIXME: report invalid UTF-8 encoding */
-    qstring_append_json_string(jov->str, *obj);
+    if (qstring_append_json_string(jov->str, *obj) < 0 && jov->strict) {
+        error_setg(errp, "string value of '%s' is not valid UTF-8",
+                   name ? name : "null");
+    }
 }

 static void json_output_type_number(Visitor *v, const char *name, double *obj,
@@ -143,14 +147,16 @@ static void json_output_type_number(Visitor *v, const 
char *name, double *obj,
     JsonOutputVisitor *jov = to_jov(v);

     json_output_name(jov, name);
-    /* FIXME: report Inf/NaN problems */
-    qstring_append_json_number(jov->str, *obj);
+    if (qstring_append_json_number(jov->str, *obj) < 0 && jov->strict) {
+        error_setg(errp, "number value of '%s' is not finite",
+                   name ? name : "null");
+    }
 }

 static void json_output_type_any(Visitor *v, const char *name, QObject **obj,
                                  Error **errp)
 {
-    qobject_visit_output(v, name, *obj);
+    qobject_visit_output(v, name, *obj, errp);
 }

 static void json_output_type_null(Visitor *v, const char *name, Error **errp)
@@ -181,12 +187,13 @@ static void json_output_free(Visitor *v)
     g_free(jov);
 }

-Visitor *json_output_visitor_new(bool pretty, char **result)
+Visitor *json_output_visitor_new(bool pretty, bool strict, char **result)
 {
     JsonOutputVisitor *v;

     v = g_malloc0(sizeof(*v));
     v->pretty = pretty;
+    v->strict = strict;
     v->result = result;
     *result = NULL;
     v->str = qstring_new();
diff --git a/qemu-img.c b/qemu-img.c
index d5dc19b..b0c6dd3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -480,7 +480,7 @@ fail:
 static void dump_json_image_check(ImageCheck *check, bool quiet)
 {
     char *str;
-    Visitor *v = json_output_visitor_new(true, &str);
+    Visitor *v = json_output_visitor_new(true, true, &str);

     visit_type_ImageCheck(v, NULL, &check, &error_abort);
     visit_complete(v, &str);
@@ -2166,7 +2166,7 @@ static void dump_snapshots(BlockDriverState *bs)
 static void dump_json_image_info_list(ImageInfoList *list)
 {
     char *str;
-    Visitor *v = json_output_visitor_new(true, &str);
+    Visitor *v = json_output_visitor_new(true, true, &str);

     visit_type_ImageInfoList(v, NULL, &list, &error_abort);
     visit_complete(v, &str);
@@ -2178,7 +2178,7 @@ static void dump_json_image_info_list(ImageInfoList *list)
 static void dump_json_image_info(ImageInfo *info)
 {
     char *str;
-    Visitor *v = json_output_visitor_new(true, &str);
+    Visitor *v = json_output_visitor_new(true, true, &str);

     visit_type_ImageInfo(v, NULL, &info, &error_abort);
     visit_complete(v, &str);
diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index 05b020a..cf81ab5 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -72,62 +72,81 @@ QObject *qobject_from_jsonf(const char *string, ...)
     return obj;
 }

+typedef struct ToJson {
+{
+    Visitor *v;
+    Error **errp;
+} ToJson;
+
 static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
 {
-    Visitor *v = opaque;
+    ToJson *s = opaque;

-    qobject_visit_output(v, key, obj);
+    if (!*s->errp) {
+        qobject_visit_output(s->v, key, obj, s->errp);
+    }
 }

 static void to_json_list_iter(QObject *obj, void *opaque)
 {
-    Visitor *v = opaque;
+    ToJson *s = opaque;

-    qobject_visit_output(v, NULL, obj);
+    if (!*s->errp) {
+        qobject_visit_output(s->v, NULL, obj, s->errp);
+    }
 }

-void qobject_visit_output(Visitor *v, const char *name, const QObject *obj)
+void qobject_visit_output(Visitor *v, const char *name, const QObject *obj,
+                          Error **errp)
 {
+    Error *err = NULL;
+    ToJson s = { .v = v, .errp = &err, };
+
     switch (qobject_type(obj)) {
     case QTYPE_QNULL:
-        visit_type_null(v, name, &error_abort);
+        visit_type_null(v, name, errp);
         break;
     case QTYPE_QINT: {
         int64_t val = qint_get_int(qobject_to_qint(obj));
-        visit_type_int64(v, name, &val, &error_abort);
+        visit_type_int64(v, name, &val, errp);
         break;
     }
     case QTYPE_QSTRING: {
         const char *str = qstring_get_str(qobject_to_qstring(obj));
-        /* FIXME: no way inform user if we modified the string to
-         * avoid encoding errors */
-        visit_type_str(v, name, (char **)&str, &error_abort);
+        visit_type_str(v, name, (char **)&str, errp);
         break;
     }
     case QTYPE_QDICT: {
         QDict *val = qobject_to_qdict(obj);
-        visit_start_struct(v, name, NULL, 0, &error_abort);
-        qdict_iter(val, to_json_dict_iter, v);
-        visit_check_struct(v, &error_abort);
+        visit_start_struct(v, name, NULL, 0, &err);
+        if (!err) {
+            qdict_iter(val, to_json_dict_iter, &s);
+        }
+        if (!err) {
+            visit_check_struct(v, &err);
+        }
+        error_propagate(errp, err);
         visit_end_struct(v, NULL);
         break;
     }
     case QTYPE_QLIST: {
         QList *val = qobject_to_qlist(obj);
-        visit_start_list(v, name, NULL, 0, &error_abort);
-        qlist_iter(val, to_json_list_iter, v);
+        visit_start_list(v, name, NULL, 0, &err);
+        if (!err) {
+            qlist_iter(val, to_json_list_iter, &s);
+        }
+        error_propagate(errp, err);
         visit_end_list(v, NULL);
         break;
     }
     case QTYPE_QFLOAT: {
         double val = qfloat_get_double(qobject_to_qfloat(obj));
-        /* FIXME: no way inform user if we generated invalid JSON */
-        visit_type_number(v, name, &val, NULL);
+        visit_type_number(v, name, &val, errp);
         break;
     }
     case QTYPE_QBOOL: {
         bool val = qbool_get_bool(qobject_to_qbool(obj));
-        visit_type_bool(v, name, &val, &error_abort);
+        visit_type_bool(v, name, &val, errp);
         break;
     }
     default:
@@ -138,9 +157,9 @@ void qobject_visit_output(Visitor *v, const char *name, 
const QObject *obj)
 QString *qobject_to_json(const QObject *obj, bool pretty)
 {
     char *str;
-    Visitor *v = json_output_visitor_new(pretty, &str);
+    Visitor *v = json_output_visitor_new(pretty, false, &str);

-    qobject_visit_output(v, NULL, obj);
+    qobject_visit_output(v, NULL, obj, &error_abort);
     visit_complete(v, &str);
     visit_free(v);
     return qstring_wrap_str(str);
@@ -222,8 +241,6 @@ int qstring_append_json_number(QString *qstring, double 
number)
     /* FIXME: snprintf() is locale dependent; but JSON requires
      * numbers to be formatted as if in the C locale. Dependence
      * on C locale is a pervasive issue in QEMU. */
-    /* FIXME: This risks printing Inf or NaN, which are not valid
-     * JSON values. */
     /* FIXME: the default precision of 6 for %f often causes
      * rounding errors; we should be using DBL_DECIMAL_DIG (17),
      * and only rounding to a shorter number if the result would
diff --git a/tests/check-qobject-json.c b/tests/check-qobject-json.c
index 267fc67..b4f3dc4 100644
--- a/tests/check-qobject-json.c
+++ b/tests/check-qobject-json.c
@@ -12,6 +12,7 @@
  */
 #include "qemu/osdep.h"
 #include <glib.h>
+#include <math.h>

 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qobject-json.h"
@@ -920,6 +921,8 @@ static void simple_number(void)
 static void float_number(void)
 {
     int i;
+    QFloat *qfloat;
+    QString *str;
     struct {
         const char *encoded;
         double decoded;
@@ -934,7 +937,6 @@ static void float_number(void)

     for (i = 0; test_cases[i].encoded; i++) {
         QObject *obj;
-        QFloat *qfloat;

         obj = qobject_from_json(test_cases[i].encoded);
         g_assert(obj != NULL);
@@ -944,8 +946,6 @@ static void float_number(void)
         g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);

         if (test_cases[i].skip == 0) {
-            QString *str;
-
             str = qobject_to_json(obj, false);
             g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
             QDECREF(str);
@@ -953,6 +953,15 @@ static void float_number(void)

         QDECREF(qfloat);
     }
+
+    /* Although Infinity/NaN are not valid JSON, we can generate them
+     * on output as an extension.
+     * FIXME: The parser doesn't handle infinity/NaN on input. */
+    qfloat = qfloat_from_double(INFINITY);
+    str = qobject_to_json(QOBJECT(qfloat), false);
+    g_assert_cmpstr(qstring_get_str(str), ==, "inf");
+    QDECREF(str);
+    QDECREF(qfloat);
 }

 static void vararg_number(void)
diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
index fd14e26..e0478bc 100644
--- a/tests/test-json-output-visitor.c
+++ b/tests/test-json-output-visitor.c
@@ -14,6 +14,7 @@

 #include "qemu/osdep.h"
 #include <glib.h>
+#include <math.h>

 #include "qemu-common.h"
 #include "qapi/json-output-visitor.h"
@@ -32,7 +33,7 @@ static void visitor_output_setup(TestOutputVisitorData *data,
                                  const void *arg)
 {
     const bool *pretty = arg;
-    data->ov = json_output_visitor_new(*pretty, &data->str);
+    data->ov = json_output_visitor_new(*pretty, true, &data->str);
     g_assert(data->ov);
     data->pretty = *pretty;
 }
@@ -89,13 +90,18 @@ static void test_visitor_out_number(TestOutputVisitorData 
*data,
 {
     double value = 3.14;
     const char *out;
+    Error *err = NULL;

     visit_type_number(data->ov, NULL, &value, &error_abort);

     out = visitor_get(data);
     g_assert_cmpstr(out, ==, "3.14");

-    /* FIXME: JSON requires finite numbers */
+    /* JSON requires finite numbers */
+    visitor_reset(data);
+    value = INFINITY;
+    visit_type_number(data->ov, NULL, &value, &err);
+    error_free_or_abort(&err);
 }

 static void test_visitor_out_string(TestOutputVisitorData *data,
@@ -103,11 +109,18 @@ static void test_visitor_out_string(TestOutputVisitorData 
*data,
 {
     char *string = (char *) "Q E M U";
     const char *out;
+    Error *err = NULL;

     visit_type_str(data->ov, NULL, &string, &error_abort);

     out = visitor_get(data);
     g_assert_cmpstr(out, ==, "\"Q E M U\"");
+
+    /* JSON requires valid UTF-8 encodings */
+    visitor_reset(data);
+    string = (char *) "\xfe";
+    visit_type_str(data->ov, NULL, &string, &err);
+    error_free_or_abort(&err);
 }

 static void test_visitor_out_enum(TestOutputVisitorData *data,
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 0db0b19..68e074f 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -17,6 +17,7 @@

 #include "qemu/osdep.h"
 #include <glib.h>
+#include <math.h>

 #include "qemu-common.h"
 #include "qapi/error.h"
@@ -97,6 +98,14 @@ static void test_visitor_out_number(TestOutputVisitorData 
*data,
     obj = visitor_get(data);
     g_assert(qobject_type(obj) == QTYPE_QFLOAT);
     g_assert(qfloat_get_double(qobject_to_qfloat(obj)) == value);
+
+    /* While JSON requires finite values, QObject does not. */
+    visitor_reset(data);
+    value = INFINITY;
+    visit_type_number(data->ov, NULL, &value, &error_abort);
+    obj = visitor_get(data);
+    g_assert(qobject_type(obj) == QTYPE_QFLOAT);
+    g_assert(qfloat_get_double(qobject_to_qfloat(obj)) == INFINITY);
 }

 static void test_visitor_out_string(TestOutputVisitorData *data,
@@ -110,6 +119,14 @@ static void test_visitor_out_string(TestOutputVisitorData 
*data,
     obj = visitor_get(data);
     g_assert(qobject_type(obj) == QTYPE_QSTRING);
     g_assert_cmpstr(qstring_get_str(qobject_to_qstring(obj)), ==, string);
+
+    /* While JSON requires valid UTF-8 encoding, QObject does not. */
+    visitor_reset(data);
+    string = (char *) "\xfe";
+    visit_type_str(data->ov, NULL, &string, &error_abort);
+    obj = visitor_get(data);
+    g_assert(qobject_type(obj) == QTYPE_QSTRING);
+    g_assert_cmpstr(qstring_get_str(qobject_to_qstring(obj)), ==, string);
 }

 static void test_visitor_out_no_string(TestOutputVisitorData *data,
-- 
2.5.5