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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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 <eblake@redhat.com>

We need to decide whether QMP should keep extending JSON for numbers
(see my review of PATCH 17), and how to treat invalid UTF-8.  If the
decision leads to two JSON dialects in QEMU, one strict and one QMP, we
need this patch.

> ---
> 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);

Switch to strict not documented in the commit message.  Is it a no-op?

More of the same below.

>
>      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);
> +        }

       for (ent = qdict_first(val); ent; ent = qdict_next(val, ent)) {
           qobject_visit_output(v, ent->key, ent->value, &err);
       }

> +        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);

               QLIST_FOREACH_ENTRY(val, ent) {
                   qobject_visit_output(s->v, NULL, obj, errp);
               }

> +        }
> +        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);

Just noticed the pre-patch difference NULL here and &error_abort
elsewhere.  I'd use &error_abort here, too [PATCH 26].

>          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. */

I think this one should be dropped in PATCH 17, where the invalid JSON
becomes a documented interface option.

>      /* 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
[...]