Re: [Qemu-devel] [PATCH v9 01/37] qobject: Document more shortcomings in our number handling

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> We've already documented that our JSON parsing is locale dependent;
> but we should also document that our JSON output has the same
> problem.  Additionally, JSON requires finite values (you have to
> upgrade to JSON5 to get support for Inf or NaN), and our output
> risks truncating floating point numbers to the point of losing
> significant precision.
>
> Sadly, this series is not going to be the one that addresses these
> problems.
>
> Fix some trailing whitespace I noticed in the vicinity.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: no change
> v7: new patch
> ---
>  qobject/json-parser.c | 4 +++-
>  qobject/qjson.c       | 8 +++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 3c5d35d..6ab98a7 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -1,5 +1,5 @@
>  /*
> - * JSON Parser 
> + * JSON Parser
>   *
>   * Copyright IBM, Corp. 2009
>   *
> @@ -519,6 +519,8 @@ static QObject *parse_literal(JSONParserContext *ctxt)
>      }
>      case JSON_FLOAT:
>          /* FIXME dependent on locale */
> +        /* FIXME our lexer matches RFC7159 in forbidding Inf or NaN,

For what it's worth, the RFC spells this "RFC 7159".

> +         * but those might be useful extensions beyond JSON */
>          return QOBJECT(qfloat_from_double(strtod(token->str, NULL)));
>      default:
>          abort();
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index a3e6a7c..41d9d65 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -237,6 +237,12 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
>          char buffer[1024];
>          int len;
>
> +        /* FIXME: snprintf() is locale dependent; but JSON requires
> +         * numbers to be formatted as if in the C locale. */

The new FIXME matches the existing one in parse_literal(), visible
above.

However, dependence on C locale is a pervasive issue in QEMU.  These two
comments could give readers the idea that it's an issue just here.
Suggest to add something like "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 %f may be insufficient to
> +         * tell this number apart from others. */

Yup.

The easy way to avoid loss of precision is %a, but of course that's way
too much sophistication for JSON.

Avoiding loss of precision with a decimal format is non-trivial; see
Steele, Jr., Guy L., and White, Jon L. How to print floating-point
numbers accurately, SIGPLAN ’90, and later improvements collected here:
http://stackoverflow.com/questions/7153979/algorithm-to-convert-an-ieee-754-double-to-a-string

>          len = snprintf(buffer, sizeof(buffer), "%f", qfloat_get_double(val));
>          while (len > 0 && buffer[len - 1] == '0') {
>              len--;
> @@ -247,7 +253,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
>          } else {
>              buffer[len] = 0;
>          }
> -        
> +
>          qstring_append(str, buffer);
>          break;
>      }