Eric Blake <ebl...@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 <ebl...@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
[...]
