[v3,04/18] qapi: Factor out JSON number formatting

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 <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@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(-)

Eric Blake <eblake@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 <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@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 <lcapitulino@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 <lcapitulino@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)

On 04/29/2016 07:22 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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 <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>

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

Okay.

> 
> "Append it but set errp" feels odd.  Normally, returning with an error
> set means the function failed to do its job.

This one's weird because by the end of the series, it will be used by
the new JSON visitor (which wants the error message because that is not
valid JSON, and doesn't care if the QString is slightly longer); as well
as the existing QMP output visitor (where existing behavior ignores that
it is not valid JSON, and we don't really have a convenient way to pass
errors back up the stack).  Is it worth trying to plumb in better error
reporting to the QMP output visitor, and/or add assertions that values
are finite, and/or document that QMP has an extension beyond JSON in
that it accepts and also might produce Inf/NaN?

> 
>> +
>> +    /* 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?

Yep. Looks like I messed up the rebase (I realized I had to re-move
updated code, but didn't scrub the comments after the move).


> 
> I think this belongs into qobject-json.c, like the previous patch's
> qstring_append_json_string().

Sounds reasonable.

Eric Blake <eblake@redhat.com> writes:

> On 04/29/2016 07:22 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@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 <eblake@redhat.com>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>
>
>>>  /**
>>> + * 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.
>
> Okay.
>
>> 
>> "Append it but set errp" feels odd.  Normally, returning with an error
>> set means the function failed to do its job.
>
> This one's weird because by the end of the series, it will be used by
> the new JSON visitor (which wants the error message because that is not
> valid JSON, and doesn't care if the QString is slightly longer); as well
> as the existing QMP output visitor (where existing behavior ignores that
> it is not valid JSON, and we don't really have a convenient way to pass
> errors back up the stack).  Is it worth trying to plumb in better error
> reporting to the QMP output visitor, and/or add assertions that values
> are finite, and/or document that QMP has an extension beyond JSON in
> that it accepts and also might produce Inf/NaN?

QMP is what it is.  We can try to get it back closer to pure JSON, and
declaring attempts to emit non-finite numbers bugs would be a step in
that direction.

What kind of bugs would that be?

If it's a programming bug, then assert.  How would a user of the QMP
output visitor avoid this programming bug?

If it's not a programming bug, then fail with error set.  How would a
user of the QMP output visitor handle this error?

That said, I'm reluctant to mess with QMP now.  We really, really need
to control the scope of your ongoing QAPI work, or we'll never get to
Marc-André's.

Instead, I'd prefer to document what we have.  If we plan to change it,
say so, and add a suitable TODO or FIXME comment.

With things kept as they are, I can see two ways to avoid
qstring_append_json_number()'s unusual behavior:

1. Add a @must_be_finite parameter.  Set the error only when it's true,
and don't do anything else then.

2. Lift the decision whether non-finite is okay into the callers.
Instead of

    // non-finite is okay
    qstring_append_json_number(qs, num, NULL);

    // non-finite is not okay
    qstring_append_json_number(qs, num, &err);
    if (err) {
        // handle error...
        // return, goto out, or similar
    }

do

    // non-finite is okay
    qstring_append_json_number(qs, num);

    // non-finite is not okay
    if (!isfinite(num) {
        error_setg(&err, "Non-finite number %f is not valid JSON", num);
        // handle error...
        // return, goto out, or similar
    }
    qstring_append_json_number(qs, num);

>>> +
>>> +    /* 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?
>
> Yep. Looks like I messed up the rebase (I realized I had to re-move
> updated code, but didn't scrub the comments after the move).
>
>
>> 
>> I think this belongs into qobject-json.c, like the previous patch's
>> qstring_append_json_string().
>
> Sounds reasonable.