Re: [Qemu-devel] [PATCH v3 04/18] qapi: Factor out JSON number formatting

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> Reviewed-by: Fam Zheng <address@hidden>
>
> ---
> 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 <address@hidden>
> @@ -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 <address@hidden>
> @@ -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)