On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote: > Tao Xu <[email protected]> writes: > > > On 11/7/2019 9:31 PM, Eduardo Habkost wrote: > >> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote: > >>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote: > >>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote: > >>>>> Add tests for time input such as zero, around limit of precision, > >>>>> signed upper limit, actual upper limit, beyond limits, time suffixes, > >>>>> and etc. > >>>>> > >>>>> Signed-off-by: Tao Xu <[email protected]> > >>>>> --- > >>>> [...] > >>>>> + /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */ > >>>>> + qdict = keyval_parse("time1=9223372036854774784," /* > >>>>> 7ffffffffffffc00 */ > >>>>> + "time2=9223372036854775295", /* > >>>>> 7ffffffffffffdff */ > >>>>> + NULL, &error_abort); > >>>>> + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); > >>>>> + qobject_unref(qdict); > >>>>> + visit_start_struct(v, NULL, NULL, 0, &error_abort); > >>>>> + visit_type_time(v, "time1", &time, &error_abort); > >>>>> + g_assert_cmphex(time, ==, 0x7ffffffffffffc00); > >>>>> + visit_type_time(v, "time2", &time, &error_abort); > >>>>> + g_assert_cmphex(time, ==, 0x7ffffffffffffc00); > >>>> > >>>> I'm confused by this test case and the one below[1]. Are these > >>>> known bugs? Shouldn't we document them as known bugs? > >>> > >>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the > >>> precision is 53 bits, so in these cases, 7ffffffffffffdff and > >>> fffffffffffffbff are rounded. > >> > >> My questions remain: why isn't this being treated like a bug? > >> > > Hi Markus, > > > > I am confused about the code here too. Because in do_strtosz(), the > > upper limit is > > > > val * mul >= 0xfffffffffffffc00 > > > > So some data near 53 bit may be rounded. Is there a bug? > > No, but the design is surprising, and the functions lack written > contracts, except for the do_strtosz() helper, which has one that sucks. > > qemu_strtosz() & friends are designed to accept fraction * unit > multiplier. Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz() > and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with > qemu_strtosz_metric(). Whether supporting fractions is a good idea is > debatable, but it's what we've got. > > The implementation limits the numeric part to the precision of double, > i.e. 53 bits. "8PiB should be enough for anybody." > > Switching it from double to long double raises the limit to the > precision of long double. At least 64 bit on common hosts, but hosts > exist where it's the same 53 bits. Do we support any such hosts? If > yes, then we'd make the precision depend on the host, which feels like a > bad idea. > > A possible alternative is to parse the numeric part both as a double and > as a 64 bit unsigned integer, then use whatever consumes more > characters. This enables providing full 64 bits unless you actually use > a fraction. >
This sounds like the right thing to do, if user input is an integer and the code in the other end is consuming an integer. > As far as I remember, the only problem we've ever had with the 53 bits > limit is developer confusion :) > Developer confusion, I can deal with. However, exposing this behavior on external interfaces is a bug to me. I don't know how serious the bug is because I don't know which interfaces are affected by it. Do we have a list? > Patches welcome. My first goal is to get the maintainers of that code to recognize it as a bug. Then I hope this will motivate somebody else to fix it. :) -- Eduardo
