| Message ID | 1463632874-28559-18-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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). (For > now, there is only one client, but later patches will use it.) > > Adding a return value will let callers that care diagnose the > situation where an attempt was made to output invalid JSON (which > does not specify Infinity or NaN). None of the current callers > care, but a future patch wants to make it possible to turn this > situation into an error. > > Signed-off-by: Eric Blake <eblake@redhat.com> [...] > @@ -303,3 +282,39 @@ int qstring_append_json_string(QString *qstring, const char *str) > qstring_append(qstring, "\""); > return result; > } > + > +/** > + * Append a JSON representation of @number to @qstring. > + * > + * Returns -1 if the added text is not strict JSON, or 0 if the number > + * was finite. > + */ Suggest: * Return 0 if the number is finite, as required by RFC 7159, else -1. The return value makes some sense only for symmetry with qstring_append_json_string(). Without that, I'd ask you to keep this function simple. Callers could just as easily test isfinite() themselves. > +int qstring_append_json_number(QString *qstring, double number) > +{ > + 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", 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); > + return isfinite(number) ? 0 : -1; > +}
On 06/02/2016 09:02 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). (For >> now, there is only one client, but later patches will use it.) >> >> Adding a return value will let callers that care diagnose the >> situation where an attempt was made to output invalid JSON (which >> does not specify Infinity or NaN). None of the current callers >> care, but a future patch wants to make it possible to turn this >> situation into an error. > > Suggest: > > * Return 0 if the number is finite, as required by RFC 7159, else -1. > > The return value makes some sense only for symmetry with > qstring_append_json_string(). Without that, I'd ask you to keep this > function simple. Callers could just as easily test isfinite() > themselves. I'm actually thinking of modifying this, given the recent thread pointing out that libvirt chokes hard on JSON extensions: https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg04398.html That is, for symmetry with qstring_append_json_string(), I'm thinking of changing NaN to 0 and Inf to DBL_MAX, and always outputting a finite value, in addition to returning -1 to inform the caller that a substitution was made, so that the output is always strict JSON.
Eric Blake <eblake@redhat.com> writes: > On 06/02/2016 09:02 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). (For >>> now, there is only one client, but later patches will use it.) >>> >>> Adding a return value will let callers that care diagnose the >>> situation where an attempt was made to output invalid JSON (which >>> does not specify Infinity or NaN). None of the current callers >>> care, but a future patch wants to make it possible to turn this >>> situation into an error. > >> >> Suggest: >> >> * Return 0 if the number is finite, as required by RFC 7159, else -1. >> >> The return value makes some sense only for symmetry with >> qstring_append_json_string(). Without that, I'd ask you to keep this >> function simple. Callers could just as easily test isfinite() >> themselves. > > I'm actually thinking of modifying this, given the recent thread > pointing out that libvirt chokes hard on JSON extensions: > > https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg04398.html > > That is, for symmetry with qstring_append_json_string(), I'm thinking of > changing NaN to 0 and Inf to DBL_MAX, and always outputting a finite > value, in addition to returning -1 to inform the caller that a > substitution was made, so that the output is always strict JSON. Mapping infinities to DBL_MIN and DBL_MAX is debatable, but mapping NaN to zero is outright wrong. If we decide QMP should stick to JSON here and avoid non-finite numbers, we need to treat an attempt to emit a non-finite number as a bug: assert(isfinite(...)). Making sure nothing ever attempts to emit such numbers will be tedious. If we decide QMP should remain as it is, we need to document non-finite numbers among its JSON extensions. We should also fix our QMP parsers to accept non-finite numbers then. Including the one in libvirt. Attempts to emit non-finite numbers then *may* be bugs. Really no different than finite numbers outside their intended range, such as a negative size. Catching these bugs is of course also tedious. The difference is that they manifest in QMP as semantic instead of lexical errors. Lexical errors are the worst to handle gracefully.
On 06/03/2016 03:02 AM, Markus Armbruster wrote: >>> Suggest: >>> >>> * Return 0 if the number is finite, as required by RFC 7159, else -1. >>> >>> The return value makes some sense only for symmetry with >>> qstring_append_json_string(). Without that, I'd ask you to keep this >>> function simple. Callers could just as easily test isfinite() >>> themselves. >> >> I'm actually thinking of modifying this, given the recent thread >> pointing out that libvirt chokes hard on JSON extensions: >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg04398.html >> >> That is, for symmetry with qstring_append_json_string(), I'm thinking of >> changing NaN to 0 and Inf to DBL_MAX, and always outputting a finite >> value, in addition to returning -1 to inform the caller that a >> substitution was made, so that the output is always strict JSON. > > Mapping infinities to DBL_MIN and DBL_MAX is debatable, but mapping NaN > to zero is outright wrong. How about this alternative: Finite values remain numbers: "number":1 But non-finite values are output as strings, so that our output is always valid JSON - the recipient may not be expecting a string in place of a number, but at least should be able to parse the output rather than choking hard. "number":"nan" The return value -1 then indicates that a stringized replacement was used, so that any later patch can use a strict flag on whether to allow the replacement output or assert. > > If we decide QMP should stick to JSON here and avoid non-finite numbers, > we need to treat an attempt to emit a non-finite number as a bug: > assert(isfinite(...)). Making sure nothing ever attempts to emit such > numbers will be tedious. > > If we decide QMP should remain as it is, we need to document non-finite > numbers among its JSON extensions. We should also fix our QMP parsers > to accept non-finite numbers then. Including the one in libvirt. > Attempts to emit non-finite numbers then *may* be bugs. Really no > different than finite numbers outside their intended range, such as a > negative size. Catching these bugs is of course also tedious. The > difference is that they manifest in QMP as semantic instead of lexical > errors. Lexical errors are the worst to handle gracefully. I may still try to tackle fixing the QMP parser to accept NaN and infinity on input (since it's hand-written, we at least have control over that) - it will certainly be easier than getting libvirt to parse non-finite numbers (libvirt uses libyajl, and my emails to the yajl mailing list have gone unanswered, making me think the project is not very vibrant and thus not very patchable). But with my proposal of producing a stringized non-finite value, we at least convert lexical into semantic errors, which I agree with your assessment is a nicer way of dealing with it. Of course, a policy change of outputting stringized non-finite numbers should be separate from refactoring patches that just move functions around.
Eric Blake <eblake@redhat.com> writes: > On 06/03/2016 03:02 AM, Markus Armbruster wrote: > >>>> Suggest: >>>> >>>> * Return 0 if the number is finite, as required by RFC 7159, else -1. >>>> >>>> The return value makes some sense only for symmetry with >>>> qstring_append_json_string(). Without that, I'd ask you to keep this >>>> function simple. Callers could just as easily test isfinite() >>>> themselves. >>> >>> I'm actually thinking of modifying this, given the recent thread >>> pointing out that libvirt chokes hard on JSON extensions: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg04398.html >>> >>> That is, for symmetry with qstring_append_json_string(), I'm thinking of >>> changing NaN to 0 and Inf to DBL_MAX, and always outputting a finite >>> value, in addition to returning -1 to inform the caller that a >>> substitution was made, so that the output is always strict JSON. >> >> Mapping infinities to DBL_MIN and DBL_MAX is debatable, but mapping NaN >> to zero is outright wrong. > > How about this alternative: > > Finite values remain numbers: > "number":1 > > But non-finite values are output as strings, so that our output is > always valid JSON - the recipient may not be expecting a string in place > of a number, but at least should be able to parse the output rather than > choking hard. > "number":"nan" > > The return value -1 then indicates that a stringized replacement was > used, so that any later patch can use a strict flag on whether to allow > the replacement output or assert. I dislike this a lot less than mapping non-finite numbers to finite ones. I still dislike it, because it defeats fitting a schema to QMP: instead of the true JSON type "number", we'd need the sum type of "number" and "string", which this really isn't: only a few special strings are valid, and they're not actually strings. If the schema language can do sum types, we'd even be stuck with their common super-type. The most practical solution isn't always a likable one, though. >> If we decide QMP should stick to JSON here and avoid non-finite numbers, >> we need to treat an attempt to emit a non-finite number as a bug: >> assert(isfinite(...)). Making sure nothing ever attempts to emit such >> numbers will be tedious. >> >> If we decide QMP should remain as it is, we need to document non-finite >> numbers among its JSON extensions. We should also fix our QMP parsers >> to accept non-finite numbers then. Including the one in libvirt. >> Attempts to emit non-finite numbers then *may* be bugs. Really no >> different than finite numbers outside their intended range, such as a >> negative size. Catching these bugs is of course also tedious. The >> difference is that they manifest in QMP as semantic instead of lexical >> errors. Lexical errors are the worst to handle gracefully. > > I may still try to tackle fixing the QMP parser to accept NaN and > infinity on input (since it's hand-written, we at least have control > over that) Making json-lexer.c recognize infinities and NaNs in strtod() syntax shouldn't be hard. I'd omit nan(n-char-sequence-opt), because its semantics are implementation defined. I'd also omit all spellings other than inf and nan. That leaves inf, +inf, -inf, nan, +nan, -nan. > - it will certainly be easier than getting libvirt to parse > non-finite numbers (libvirt uses libyajl, and my emails to the yajl > mailing list have gone unanswered, making me think the project is not > very vibrant and thus not very patchable). Nobody likes to carry downstream patches, but an unresponsive upstream may leave you no choice. > But with my proposal of > producing a stringized non-finite value, we at least convert lexical > into semantic errors, which I agree with your assessment is a nicer way > of dealing with it. > > Of course, a policy change of outputting stringized non-finite numbers > should be separate from refactoring patches that just move functions around. Yes.
On 06/13/2016 02:22 AM, Markus Armbruster wrote: >> I may still try to tackle fixing the QMP parser to accept NaN and >> infinity on input (since it's hand-written, we at least have control >> over that) > > Making json-lexer.c recognize infinities and NaNs in strtod() syntax > shouldn't be hard. I'd omit nan(n-char-sequence-opt), because its > semantics are implementation defined. I'd also omit all spellings other > than inf and nan. That leaves inf, +inf, -inf, nan, +nan, -nan. +inf and +nan weren't worth it (JSON doesn't accept +0 either), but '-inf' and '-nan' were easy. In fact, '-infinity' was easy too - see the posted patches: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02652.html and you are right that it was not worth 'nan(n-char-seq)'. > >> - it will certainly be easier than getting libvirt to parse >> non-finite numbers (libvirt uses libyajl, and my emails to the yajl >> mailing list have gone unanswered, making me think the project is not >> very vibrant and thus not very patchable). > > Nobody likes to carry downstream patches, but an unresponsive upstream > may leave you no choice. Another (possibly-ugly) option that I thought of over the weekend: We have 'qmp_capabilities' for handshakes between client and server. The server could advertise a new capability 'nonfinite' in its initial greeting, and if the client replies with the same capability, then the server knows that the client is prepared to accept bareword non-finite numbers. A client that doesn't see the server advertise anything has no guarantees, and a server that knows the capability but doesn't see the client request the capability should avoid sending bareword non-finite.
Eric Blake <eblake@redhat.com> writes: > On 06/13/2016 02:22 AM, Markus Armbruster wrote: > >>> I may still try to tackle fixing the QMP parser to accept NaN and >>> infinity on input (since it's hand-written, we at least have control >>> over that) >> >> Making json-lexer.c recognize infinities and NaNs in strtod() syntax >> shouldn't be hard. I'd omit nan(n-char-sequence-opt), because its >> semantics are implementation defined. I'd also omit all spellings other >> than inf and nan. That leaves inf, +inf, -inf, nan, +nan, -nan. > > +inf and +nan weren't worth it (JSON doesn't accept +0 either), but > '-inf' and '-nan' were easy. In fact, '-infinity' was easy too - see > the posted patches: > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02652.html > and you are right that it was not worth 'nan(n-char-seq)'. > >> >>> - it will certainly be easier than getting libvirt to parse >>> non-finite numbers (libvirt uses libyajl, and my emails to the yajl >>> mailing list have gone unanswered, making me think the project is not >>> very vibrant and thus not very patchable). >> >> Nobody likes to carry downstream patches, but an unresponsive upstream >> may leave you no choice. > > Another (possibly-ugly) option that I thought of over the weekend: We > have 'qmp_capabilities' for handshakes between client and server. The > server could advertise a new capability 'nonfinite' in its initial > greeting, and if the client replies with the same capability, then the > server knows that the client is prepared to accept bareword non-finite > numbers. A client that doesn't see the server advertise anything has no > guarantees, and a server that knows the capability but doesn't see the > client request the capability should avoid sending bareword non-finite. The capability negotiation mechanism exists to support things like that. However, the problem what to do when the command reply contains non-finite numbers remains. A capability can at best hide whatever ugly solution we have for that, and perhaps help us get rid of it in an orderly fashion eventually, by refusing to talk to clients that don't accept the capability. Moreover, we're already sending non-finite numbers! Changing QMP to send something else unless the client opts into non-finite numbers via capability "nonfinite" is an incompatible change. We could weasel out of that by declaring our sending of them a bug, then turn around and declare it a feature when capability "nonfinite" is accepted. Meh.
diff --git a/include/qapi/qmp/qobject-json.h b/include/qapi/qmp/qobject-json.h index aa8ddd7..0749e7e 100644 --- a/include/qapi/qmp/qobject-json.h +++ b/include/qapi/qmp/qobject-json.h @@ -25,5 +25,6 @@ QString *qobject_to_json(const QObject *obj); QString *qobject_to_json_pretty(const QObject *obj); int qstring_append_json_string(QString *qstring, const char *str); +int qstring_append_json_number(QString *qstring, double number); #endif /* QJSON_H */ diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c index bc3634f..08daeed 100644 --- a/qobject/qobject-json.c +++ b/qobject/qobject-json.c @@ -18,6 +18,7 @@ #include "qapi/qmp/qobject-json.h" #include "qemu/unicode.h" #include "qapi/qmp/types.h" +#include <math.h> typedef struct JSONParsingState { @@ -180,30 +181,8 @@ 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 inform user if we generated invalid JSON */ + qstring_append_json_number(str, qfloat_get_double(val)); break; } case QTYPE_QBOOL: { @@ -303,3 +282,39 @@ int qstring_append_json_string(QString *qstring, const char *str) qstring_append(qstring, "\""); return result; } + +/** + * Append a JSON representation of @number to @qstring. + * + * Returns -1 if the added text is not strict JSON, or 0 if the number + * was finite. + */ +int qstring_append_json_number(QString *qstring, double number) +{ + 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", 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); + return isfinite(number) ? 0 : -1; +}
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). (For now, there is only one client, but later patches will use it.) Adding a return value will let callers that care diagnose the situation where an attempt was made to output invalid JSON (which does not specify Infinity or NaN). None of the current callers care, but a future patch wants to make it possible to turn this situation into an error. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: keep helper in qobject-json.[ch], don't use Error **errp for inf/NaN handling, drop R-b 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/qobject-json.h | 1 + qobject/qobject-json.c | 63 +++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 24 deletions(-)