Eric Blake <ebl...@redhat.com> writes:
> Pull out a new qstring_append_json_number() helper, so that all
> JSON output producers can use a consistent style for printing
> floating point without duplicating code (since we are doing more
> data massaging than a simple printf format can handle).
>
> Address one FIXME by adding an Error parameter and warning the
> caller if the requested number cannot be represented in JSON;
> but add another FIXME in its place because we have no way to
> report the problem higher up the stack.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> Reviewed-by: Fam Zheng <f...@redhat.com>
>
> ---
> v3: rebase to latest; even though the patch differs quite a bit
> from v2, the changes are due to prior comment changes that are
> just moving between files, so R-b kept
> v2: minor wording tweaks
> ---
> include/qapi/qmp/qstring.h | 4 +++-
> qobject/qobject-json.c | 27 +++------------------------
> qobject/qstring.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index a254ee3..f00e3df 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -1,7 +1,7 @@
> /*
> * QString Module
> *
> - * Copyright (C) 2009 Red Hat Inc.
> + * Copyright (C) 2009, 2015 Red Hat Inc.
> *
> * Authors:
> * Luiz Capitulino <lcapitul...@redhat.com>
> @@ -14,6 +14,7 @@
> #define QSTRING_H
>
> #include "qapi/qmp/qobject.h"
> +#include "qapi/error.h"
>
> typedef struct QString {
> QObject base;
> @@ -31,6 +32,7 @@ void qstring_append_int(QString *qstring, int64_t value);
> void qstring_append(QString *qstring, const char *str);
> void qstring_append_chr(QString *qstring, int c);
> void qstring_append_json_string(QString *qstring, const char *raw);
> +void qstring_append_json_number(QString *qstring, double number, Error
> **errp);
> QString *qobject_to_qstring(const QObject *obj);
> void qstring_destroy_obj(QObject *obj);
>
> diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
> index 6fed1ee..97bccb7 100644
> --- a/qobject/qobject-json.c
> +++ b/qobject/qobject-json.c
> @@ -177,30 +177,9 @@ static void to_json(const QObject *obj, QString *str,
> int pretty, int indent)
> }
> case QTYPE_QFLOAT: {
> QFloat *val = qobject_to_qfloat(obj);
> - char buffer[1024];
> - int len;
> -
> - /* 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
> - * still produce the same floating point value. */
> - len = snprintf(buffer, sizeof(buffer), "%f", qfloat_get_double(val));
> - while (len > 0 && buffer[len - 1] == '0') {
> - len--;
> - }
> -
> - if (len && buffer[len - 1] == '.') {
> - buffer[len - 1] = 0;
> - } else {
> - buffer[len] = 0;
> - }
> -
> - qstring_append(str, buffer);
> + /* FIXME: no way to report invalid JSON to caller, so for now
> + * we just ignore it */
> + qstring_append_json_number(str, qfloat_get_double(val), NULL);
> break;
> }
> case QTYPE_QBOOL: {
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 9f0df5b..618dd8f 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -1,7 +1,7 @@
> /*
> * QString Module
> *
> - * Copyright (C) 2009 Red Hat Inc.
> + * Copyright (C) 2009, 2015-2016 Red Hat Inc.
> *
> * Authors:
> * Luiz Capitulino <lcapitul...@redhat.com>
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qstring.h"
> #include "qemu-common.h"
> #include "qemu/unicode.h"
> +#include <math.h>
>
> /**
> * qstring_new(): Create a new empty QString
> @@ -167,6 +168,45 @@ void qstring_append_json_string(QString *qstring, const
> char *raw)
> }
>
> /**
> + * qstring_append_json_number(): Append a JSON number to a QString.
> + * Set @errp if the number is not representable in JSON, but append the
> + * output anyway (callers can then choose to ignore the warning).
> + */
> +void qstring_append_json_number(QString *qstring, double number, Error
> **errp)
> +{
> + char buffer[1024];
> + int len;
> +
> + /* JSON does not allow Inf or NaN; append it but set errp */
> + if (!isfinite(number)) {
> + error_setg(errp, "Non-finite number %f is not valid JSON", number);
> + }
Separate patch, please.
"Append it but set errp" feels odd. Normally, returning with an error
set means the function failed to do its job.
> +
> + /* 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. */
Your !isfinite() conditional addresses this, doesn't it?
> + /* 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
> + * still produce the same floating point value. */
> + len = snprintf(buffer, sizeof(buffer), "%f", number);
> + assert(len > 0 && len < sizeof(buffer));
> + while (len > 0 && buffer[len - 1] == '0') {
> + len--;
> + }
> +
> + if (len && buffer[len - 1] == '.') {
> + buffer[len - 1] = 0;
> + } else {
> + buffer[len] = 0;
> + }
> +
> + qstring_append(qstring, buffer);
> +}
I think this belongs into qobject-json.c, like the previous patch's
qstring_append_json_string().
> +
> +/**
> * qobject_to_qstring(): Convert a QObject to a QString
> */
> QString *qobject_to_qstring(const QObject *obj)
