From f917eed3069640f6fa15f07cc5a61ecf4270e6a3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 10 Dec 2020 17:14:48 +0100 Subject: [PATCH] qobject: Fix qnum_to_string() to use sufficient precision 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 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); }