[v4,29/28] qapi: Add strict mode to JSON output visitor

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>

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

On 05/19/2016 10:52 AM, Eric Blake wrote:
> 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>
> 

> +++ b/qobject/qobject-json.c
> @@ -72,62 +72,81 @@ QObject *qobject_from_jsonf(const char *string, ...)
>      return obj;
>  }
> 
> +typedef struct ToJson {
> +{
> +    Visitor *v;

Uggh, posted the wrong version. checkpatch complained the { was on the
wrong line, so I added it in the right one and then pushed before
re-saving after deleting the duplicate one.

Actually, if this patch is worthwhile, I'm thinking that in v5, I'll
minimize some of the churn and keep the ToJsonIterState struct that I
removed in 26/28, even if it only has a single 'Visitor *v' member at
that time, rather than completely removing it there to reinstate it here.

Eric Blake <eblake@redhat.com> writes:

> On 05/19/2016 10:52 AM, Eric Blake wrote:
>> 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>
>> 
>
>> +++ b/qobject/qobject-json.c
>> @@ -72,62 +72,81 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>      return obj;
>>  }
>> 
>> +typedef struct ToJson {
>> +{
>> +    Visitor *v;
>
> Uggh, posted the wrong version. checkpatch complained the { was on the
> wrong line, so I added it in the right one and then pushed before
> re-saving after deleting the duplicate one.
>
> Actually, if this patch is worthwhile, I'm thinking that in v5, I'll
> minimize some of the churn and keep the ToJsonIterState struct that I
> removed in 26/28, even if it only has a single 'Visitor *v' member at
> that time, rather than completely removing it there to reinstate it here.

I suspect you won't need it if you replace qdict_iter() by
qdict_first(), qdict_next(), and qlist_iter() by QLIST_FOREACH_ENTRY.

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