[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH 0/3] Improve do_strtosz precision

From: Eric Blake
Subject: [PATCH 0/3] Improve do_strtosz precision
Date: Thu, 4 Feb 2021 13:07:05 -0600

Recently, commit 8b1170012b tweaked the maximum size the block layer
will allow, which in turn affects nbdkit's testsuite of edge-case
behaviors, where Rich noted [1] that our use of double meant rounding
errors that cause spurious failures in qemu-io (among other places).
So I decided to fix that.  In the process, I was reminded that we have
attempted this before, most recently in Dec 2019 [2], where Markus
(rightly, IMHO) said that any approach that tries to reimplement
strtoull() or which compares the amount of data consumed between
strtod() and strtoull() adds more complexity than it solves.

So first, our analysis of what we absolutely need:
- Existing clients expect decimal scaling to work (1M is a lot easier
  to type than 1048576)
- Existing clients expect hex to work (my initial attempt disabled it,
  and promptly hung in iotests when things like 0x10000 got parsed as
  zero rather than their intended byte value) (although our existing
  testsuite coverage was only via iotests, rather than unit tests)
- Existing clients expect sane decimal fractions to work (1.5k is
  easier to type than 1536), although our testsuite coverage of this
  was less apparant
- Existing clients are surprised when something that looks like a byte
  value is truncated or rounded due to an intermediate pass through
  double (hence Rich's bug report)

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00812.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00852.html

My solution, instead of parsing twice and picking the longest, is to
always parse the integral portion with strtoull(), then special case
on what is left: if it is 'x' or 'X', the user wanted hex (and does
NOT want a fraction, and in fact we can optionally warn about the use
of scaling suffixes in this case); if it is '.', the user wanted the
convenience of a floating point adjustment which we can do via
strtod() on the suffix (and where we can optionally warn about
fractions that are not evenly divisible into bytes).  I also enhanced
the testsuite (our coverage for hex constants was implicit via
iotests, and now explicit in the unit test; and our testsuite did not
flag the fact that we were previously accepting nonsense like '0x1.8k'
for 1536, or '1.5e1M' for 53477376).

We may decide that one or both of patch 2 and 3 goes too far, which is
why I split them out from the main enhancement.

Eric Blake (3):
  utils: Improve qemu_strtosz() to have 64 bits of precision
  utils: Deprecate hex-with-suffix sizes
  utils: Deprecate inexact fractional suffix sizes

 docs/system/deprecated.rst |  8 ++++
 tests/test-cutils.c        | 83 ++++++++++++++++++++++++++++---------
 tests/test-keyval.c        | 28 +++++++++----
 tests/test-qemu-opts.c     | 24 +++++++----
 util/cutils.c              | 85 +++++++++++++++++++++++++++++---------
 tests/qemu-iotests/049.out |  7 +++-
 6 files changed, 178 insertions(+), 57 deletions(-)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]