[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 09/33] qobject: Fix qnum_to_string() to use sufficient precision
From: |
Markus Armbruster |
Subject: |
[PULL 09/33] qobject: Fix qnum_to_string() to use sufficient precision |
Date: |
Sat, 19 Dec 2020 11:55:08 +0100 |
We should serialize numbers to JSON so that they deserialize back to
the same number. We fail to do so.
The culprit is qnum_to_string(): it uses format %f with trailing '0'
trimmed. Results in pretty output for "nice" numbers, but is prone to
nasty rounding errors. For instance, numbers between 0 and 0.0000005
get flushed to zero.
Where exactly the incorrect rounding can bite is tiresome to gauge.
Here's my take.
* In QMP output, type 'number':
- query-blockstats value avg_rd_queue_depth
- QMP query-migrate values mbps, cache-miss-rate, encoding-rate,
busy-rate, compression-rate.
Relatively harmless, I guess.
* In tracing QMP input. Harmless.
* In qemu-ga output, type 'number': guest-get-users value login-time.
Harmless.
* In output of HMP qom-get. Harmless.
Not affected, because double values don't actually occur there (I
think):
* QMP output, type 'any':
* qom-get value
* qom-list, qom-list-properties value default-value
* query-cpu-model-comparison, query-cpu-model-baseline,
query-cpu-model-expansion value props.
* qemu-img --output json output.
* "json:" pseudo-filenames generated by bdrv_refresh_filename().
* The rbd block driver's "=keyvalue-pairs" hack.
* In -object help on property default values. Aside: use of JSON
feels inappropriate here.
* Output of HMP qom-get.
* Argument conversion to QemuOpts for qdev_device_add() and HMP with
qemu_opts_from_qdict()
QMP and HMP device_add, virtio-net failover primary creation,
xen-usb "usb-host" creation, HMP netdev_add, object_add.
* The uses of qobject_input_visitor_new_flat_confused()
As far as I can tell, none of the visited types contain double
values.
* Dumping ImageInfoSpecific with dump_qobject()
Fix by formatting with %.17g. 17 decimal digits always suffice for
IEEE double.
The change to expected test output illustrates the effect: the
rounding errors are gone, but some seemingly "nice" numbers now get
converted to not so nice strings, e.g. 0.42 to "0.41999999999999998".
This is because 0.42 is not representable exactly in double. It's
more accurate in this example than strictly necessary, though.
If ugly accuracy bothers us, we can we can try using the least number
of digits that still converts back to the same double. In this
example, "0.42" would do.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20201210161452.2813491-7-armbru@redhat.com>
---
qobject/qnum.c | 24 +++---------------------
tests/check-qjson.c | 8 ++++----
tests/check-qnum.c | 4 ++--
3 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 7012fc57f2..bf1240ecec 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -161,37 +161,19 @@ double qnum_get_double(QNum *qn)
char *qnum_to_string(QNum *qn)
{
- char *buffer;
- int len;
-
switch (qn->kind) {
case QNUM_I64:
return g_strdup_printf("%" PRId64, qn->u.i64);
case QNUM_U64:
return g_strdup_printf("%" PRIu64, qn->u.u64);
case QNUM_DOUBLE:
- /* FIXME: snprintf() is locale dependent; but JSON requires
+ /* FIXME: g_strdup_printf() 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. */
- buffer = g_strdup_printf("%f" , qn->u.dbl);
- len = strlen(buffer);
- while (len > 0 && buffer[len - 1] == '0') {
- len--;
- }
-
- if (len && buffer[len - 1] == '.') {
- buffer[len - 1] = 0;
- } else {
- buffer[len] = 0;
- }
-
- return buffer;
+ /* 17 digits suffice for IEEE double */
+ return g_strdup_printf("%.17g", qn->u.dbl);
}
assert(0);
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 98515b1fd6..ca8fb816e9 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -882,10 +882,10 @@ static void float_number(void)
} test_cases[] = {
{ "32.43", 32.43 },
{ "0.222", 0.222 },
- { "-32.12313", -32.12313 },
- { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },
- { "18446744073709551616", 0x1p64 },
- { "-9223372036854775809", -0x1p63, "-9223372036854775808" },
+ { "-32.12313", -32.12313, "-32.123130000000003" },
+ { "-32.20e-10", -32.20e-10, "-3.22e-09" },
+ { "18446744073709551616", 0x1p64, "1.8446744073709552e+19" },
+ { "-9223372036854775809", -0x1p63, "-9.2233720368547758e+18" },
{},
};
int i;
diff --git a/tests/check-qnum.c b/tests/check-qnum.c
index a73809d021..b85fca2302 100644
--- a/tests/check-qnum.c
+++ b/tests/check-qnum.c
@@ -147,13 +147,13 @@ static void qnum_to_string_test(void)
qn = qnum_from_double(0.42);
tmp = qnum_to_string(qn);
- g_assert_cmpstr(tmp, ==, "0.42");
+ g_assert_cmpstr(tmp, ==, "0.41999999999999998");
g_free(tmp);
qobject_unref(qn);
qn = qnum_from_double(2.718281828459045);
tmp = qnum_to_string(qn);
- g_assert_cmpstr(tmp, ==, "2.718282"); /* BUG */
+ g_assert_cmpstr(tmp, ==, "2.7182818284590451");
g_free(tmp);
qobject_unref(qn);
}
--
2.26.2
- [PULL 00/33] QAPI patches patches for 2020-12-19, Markus Armbruster, 2020/12/19
- [PULL 04/33] tests/check-qjson: Don't skip funny QNumber to JSON conversions, Markus Armbruster, 2020/12/19
- [PULL 01/33] rocker: Revamp fp_port_get_info, Markus Armbruster, 2020/12/19
- [PULL 11/33] string-output-visitor: Fix to use sufficient precision, Markus Armbruster, 2020/12/19
- [PULL 05/33] tests/check-qjson: Examine QNum more thoroughly, Markus Armbruster, 2020/12/19
- [PULL 09/33] qobject: Fix qnum_to_string() to use sufficient precision,
Markus Armbruster <=
- [PULL 06/33] tests/check-qjson: Cover number 2^63, Markus Armbruster, 2020/12/19
- [PULL 10/33] test-string-output-visitor: Cover "unround" number, Markus Armbruster, 2020/12/19
- [PULL 08/33] tests/check-qnum: Cover qnum_to_string() for "unround" argument, Markus Armbruster, 2020/12/19
- [PULL 12/33] test-visitor-serialization: Drop insufficient precision workaround, Markus Armbruster, 2020/12/19
- [PULL 07/33] tests/check-qjson: Replace redundant large_number(), Markus Armbruster, 2020/12/19
- [PULL 02/33] migration: Refactor migrate_cap_add, Markus Armbruster, 2020/12/19
- [PULL 26/33] qobject: Drop qstring_get_try_str(), Markus Armbruster, 2020/12/19
- [PULL 13/33] test-visitor-serialization: Clean up test_primitives(), Markus Armbruster, 2020/12/19
- [PULL 19/33] Revert "qstring: add qstring_free()", Markus Armbruster, 2020/12/19
- [PULL 16/33] qobject: Make qobject_to_json_pretty() take a pretty argument, Markus Armbruster, 2020/12/19