[PATCH v2 0/4] improve do_strtosz precision

[PATCH v2 0/4] improve do_strtosz precision

[Eric Blake]

Parsing sizes with only 53 bits of precision is surprising; it's time
to fix it to use a full 64 bits of precision.

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01800.html

Since then:
- split testsuite improvements from code changes [Vladimir]
- more tests for more corner cases [Vladimir, Rich, Dan]
- fix handling of '123-45' when endptr is non-NULL [Vladimir]
- fix handling of '1.k'
- actually enable deprecation of '0x1k' [Vladimir]
- include missing deprecation text for rounded fractions
- improved commit messages

I'm still not sure I like patch 4, but it's at least worth considering.

Eric Blake (4):
  utils: Enhance testsuite for do_strtosz()
  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       |  17 ++++
 tests/test-cutils.c              | 168 ++++++++++++++++++++++++++-----
 tests/test-keyval.c              |  39 ++++---
 tests/test-qemu-opts.c           |  37 ++++---
 util/cutils.c                    | 103 +++++++++++++++----
 tests/qemu-iotests/049.out       |  14 ++-
 tests/qemu-iotests/178.out.qcow2 |   3 +-
 tests/qemu-iotests/178.out.raw   |   3 +-
 8 files changed, 305 insertions(+), 79 deletions(-)

-- 
2.30.1

[PATCH v2 1/4] utils: Enhance testsuite for do_strtosz()

[Eric Blake]

Enhance our testsuite coverage of do_strtosz() to cover some things we
know that existing users want to continue working (hex bytes), as well
as some things that accidentally work but shouldn't (hex fractions) or
accidentally fail but that users want to work (64-bit precision on
byte values).  This includes fixing a typo in the comment regarding
our parsing near 2^64.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-cutils.c | 154 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 143 insertions(+), 11 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 1aa8351520ae..6d9802e00b1b 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1960,11 +1960,19 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(res, ==, 0);
     g_assert(endptr == str + 1);

-    str = "12345";
+    /* Leading 0 gives decimal results, not octal */
+    str = "08";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 8);
+    g_assert(endptr == str + 2);
+
+    /* Leading space is ignored */
+    str = " 12345";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, 12345);
-    g_assert(endptr == str + 5);
+    g_assert(endptr == str + 6);

     err = qemu_strtosz(str, NULL, &res);
     g_assert_cmpint(err, ==, 0);
@@ -1984,7 +1992,7 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(res, ==, 0x20000000000000);
     g_assert(endptr == str + 16);

-    str = "9007199254740993"; /* 2^53+1 */
+    str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, 0x20000000000000); /* rounded to 53 bits */
@@ -1996,14 +2004,42 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(res, ==, 0xfffffffffffff800);
     g_assert(endptr == str + 20);

-    str = "18446744073709550591"; /* 0xfffffffffffffbff */
+    str = "18446744073709550591"; /* 0xfffffffffffffbff FIXME */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */
     g_assert(endptr == str + 20);

-    /* 0x7ffffffffffffe00..0x7fffffffffffffff get rounded to
-     * 0x8000000000000000, thus -ERANGE; see test_qemu_strtosz_erange() */
+    /*
+     * FIXME 0xfffffffffffffe00..0xffffffffffffffff get rounded to
+     * 2^64, thus -ERANGE; see test_qemu_strtosz_erange()
+     */
+}
+
+static void test_qemu_strtosz_hex(void)
+{
+    const char *str;
+    const char *endptr;
+    int err;
+    uint64_t res = 0xbaadf00d;
+
+    str = "0x0";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0);
+    g_assert(endptr == str + 3);
+
+    str = "0xab";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 171);
+    g_assert(endptr == str + 4);
+
+    str = "0xae";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 174);
+    g_assert(endptr == str + 4);
 }

 static void test_qemu_strtosz_units(void)
@@ -2064,14 +2100,36 @@ static void test_qemu_strtosz_units(void)

 static void test_qemu_strtosz_float(void)
 {
-    const char *str = "12.345M";
+    const char *str;
     int err;
     const char *endptr;
     uint64_t res = 0xbaadf00d;

+    str = "0.5E";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 12.345 * MiB);
+    g_assert_cmpint(res, ==, EiB / 2);
+    g_assert(endptr == str + 4);
+
+    /* For convenience, a fraction of 0 is tolerated even on bytes */
+    str = "1.0B";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1);
+    g_assert(endptr == str + 4);
+
+    /* An empty fraction is tolerated */
+    str = "1.k";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1024);
+    g_assert(endptr == str + 3);
+
+    /* For convenience, we permit values that are not byte-exact */
+    str = "12.345M";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));
     g_assert(endptr == str + 7);
 }

@@ -2106,6 +2164,47 @@ static void test_qemu_strtosz_invalid(void)
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
     g_assert(endptr == str);
+
+    /* Fractional values require scale larger than bytes */
+    /* FIXME endptr shouldn't move on -EINVAL */
+    str = "1.1B";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str + 4);
+
+    /* FIXME endptr shouldn't move on -EINVAL */
+    str = "1.1";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str + 3);
+
+    /* FIXME we should reject floating point exponents */
+    str = "1.5e1k";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 15360);
+    g_assert(endptr == str + 6);
+
+    /* FIXME we should reject floating point exponents */
+    str = "1.5E+0k";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1536);
+    g_assert(endptr == str + 7);
+
+    /* FIXME we should reject hex fractions */
+    str = "0x1.8k";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1536);
+    g_assert(endptr == str + 6);
+
+    /* FIXME we reject all other attempts at negative, why not -0 */
+    str = "-0";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0);
+    g_assert(endptr == str + 2);
 }

 static void test_qemu_strtosz_trailing(void)
@@ -2131,6 +2230,30 @@ static void test_qemu_strtosz_trailing(void)

     err = qemu_strtosz(str, NULL, &res);
     g_assert_cmpint(err, ==, -EINVAL);
+
+    str = "0x";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(res, ==, 0);
+    g_assert(endptr == str + 1);
+
+    err = qemu_strtosz(str, NULL, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+
+    str = "0.NaN";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert(endptr == str + 2);
+
+    err = qemu_strtosz(str, NULL, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+
+    str = "123-45";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(res, ==, 123);
+    g_assert(endptr == str + 3);
+
+    err = qemu_strtosz(str, NULL, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
 }

 static void test_qemu_strtosz_erange(void)
@@ -2145,12 +2268,12 @@ static void test_qemu_strtosz_erange(void)
     g_assert_cmpint(err, ==, -ERANGE);
     g_assert(endptr == str + 2);

-    str = "18446744073709550592"; /* 0xfffffffffffffc00 */
+    str = "18446744073709550592"; /* 0xfffffffffffffc00 FIXME accept this */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -ERANGE);
     g_assert(endptr == str + 20);

-    str = "18446744073709551615"; /* 2^64-1 */
+    str = "18446744073709551615"; /* 2^64-1 FIXME accept this */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -ERANGE);
     g_assert(endptr == str + 20);
@@ -2168,15 +2291,22 @@ static void test_qemu_strtosz_erange(void)

 static void test_qemu_strtosz_metric(void)
 {
-    const char *str = "12345k";
+    const char *str;
     int err;
     const char *endptr;
     uint64_t res = 0xbaadf00d;

+    str = "12345k";
     err = qemu_strtosz_metric(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, 12345000);
     g_assert(endptr == str + 6);
+
+    str = "12.345M";
+    err = qemu_strtosz_metric(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 12345000);
+    g_assert(endptr == str + 7);
 }

 int main(int argc, char **argv)
@@ -2443,6 +2573,8 @@ int main(int argc, char **argv)

     g_test_add_func("/cutils/strtosz/simple",
                     test_qemu_strtosz_simple);
+    g_test_add_func("/cutils/strtosz/hex",
+                    test_qemu_strtosz_hex);
     g_test_add_func("/cutils/strtosz/units",
                     test_qemu_strtosz_units);
     g_test_add_func("/cutils/strtosz/float",
-- 
2.30.1

Re: [PATCH v2 1/4] utils: Enhance testsuite for do_strtosz()

[Daniel P. Berrangé]

On Thu, Feb 11, 2021 at 02:44:35PM -0600, Eric Blake wrote:
> Enhance our testsuite coverage of do_strtosz() to cover some things we
> know that existing users want to continue working (hex bytes), as well
> as some things that accidentally work but shouldn't (hex fractions) or
> accidentally fail but that users want to work (64-bit precision on
> byte values).  This includes fixing a typo in the comment regarding
> our parsing near 2^64.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-cutils.c | 154 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 143 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 2/4] utils: Improve qemu_strtosz() to have 64 bits of precision

[Eric Blake]

We have multiple clients of qemu_strtosz (qemu-io, the opts visitor,
the keyval visitor), and it gets annoying that edge-case testing is
impacted by implicit rounding to 53 bits of precision due to parsing
with strtod().  As an example posted by Rich Jones:
 $ nbdkit memory $(( 2**63 - 2**30 )) --run \
   'build/qemu-io -f raw "$uri" -c "w -P 3 $(( 2**63 - 2**30 - 512 )) 512" '
 write failed: Input/output error

because 9223372035781033472 got rounded to 0x7fffffffc0000000 which is
out of bounds.

It is also worth noting that our existing parser, by virtue of using
strtod(), accepts decimal AND hex numbers, even though test-cutils
previously lacked any coverage of the latter until the previous patch.
We do have existing clients that expect a hex parse to work (for
example, iotest 33 using qemu-io -c "write -P 0xa 0x200 0x400"), but
strtod() parses "08" as 8 rather than as an invalid octal number, so
we know there are no clients that depend on octal.  Our use of
strtod() also means that "0x1.8k" would actually parse as 1536 (the
fraction is 8/16), rather than 1843 (if the fraction were 8/10); but
as this was not covered in the testsuite, I have no qualms forbidding
hex fractions as invalid, so this patch declares that the use of
fractions is only supported with decimal input, and enhances the
testsuite to document that.

Our previous use of strtod() meant that -1 parsed as a negative; now
that we parse with strtoull(), negative values can wrap around modulo
2^64, so we have to explicitly check whether the user passed in a '-';
and make it consistent to also reject '-0'.  This has the minor effect
of treating negative values as EINVAL (with no change to endptr)
rather than ERANGE (with endptr advanced to what was parsed), visible
in the updated iotest output.

We also had no testsuite coverage of "1.1e0k", which happened to parse
under strtod() but is unlikely to occur in practice; as long as we are
making things more robust, it is easy enough to reject the use of
exponents in a strtod parse.

The fix is done by breaking the parse into an integer prefix (no loss
in precision), rejecting negative values (since we can no longer rely
on strtod() to do that), determining if a decimal or hexadecimal parse
was intended (with the new restriction that a fractional hex parse is
not allowed), and where appropriate, using a floating point fractional
parse (where we also scan to reject use of exponents in the fraction).
The bulk of the patch is then updates to the testsuite to match our
new precision, as well as adding new cases we reject (whether they
were rejected or inadvertently accepted before).

Signed-off-by: Eric Blake <eblake@redhat.com>

---

Note that this approach differs from what has been attempted in the
past; see the thread starting at
https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00852.html
That approach tried to parse both as strtoull and strtod and take
whichever was longer, but that was harder to document.
---
 tests/test-cutils.c              | 74 ++++++++++----------------
 tests/test-keyval.c              | 35 +++++++++----
 tests/test-qemu-opts.c           | 33 ++++++++----
 util/cutils.c                    | 90 ++++++++++++++++++++++++--------
 tests/qemu-iotests/049.out       | 14 +++--
 tests/qemu-iotests/178.out.qcow2 |  3 +-
 tests/qemu-iotests/178.out.raw   |  3 +-
 7 files changed, 158 insertions(+), 94 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 6d9802e00b1b..bad3a6099389 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1978,8 +1978,6 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, 12345);

-    /* Note: precision is 53 bits since we're parsing with strtod() */
-
     str = "9007199254740991"; /* 2^53-1 */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
@@ -1992,10 +1990,10 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(res, ==, 0x20000000000000);
     g_assert(endptr == str + 16);

-    str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */
+    str = "9007199254740993"; /* 2^53+1 */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0x20000000000000); /* rounded to 53 bits */
+    g_assert_cmpint(res, ==, 0x20000000000001);
     g_assert(endptr == str + 16);

     str = "18446744073709549568"; /* 0xfffffffffffff800 (53 msbs set) */
@@ -2004,16 +2002,17 @@ static void test_qemu_strtosz_simple(void)
     g_assert_cmpint(res, ==, 0xfffffffffffff800);
     g_assert(endptr == str + 20);

-    str = "18446744073709550591"; /* 0xfffffffffffffbff FIXME */
+    str = "18446744073709550591"; /* 0xfffffffffffffbff */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */
+    g_assert_cmpint(res, ==, 0xfffffffffffffbff);
     g_assert(endptr == str + 20);

-    /*
-     * FIXME 0xfffffffffffffe00..0xffffffffffffffff get rounded to
-     * 2^64, thus -ERANGE; see test_qemu_strtosz_erange()
-     */
+    str = "18446744073709551615"; /* 0xffffffffffffffff */
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0xffffffffffffffff);
+    g_assert(endptr == str + 20);
 }

 static void test_qemu_strtosz_hex(void)
@@ -2166,45 +2165,43 @@ static void test_qemu_strtosz_invalid(void)
     g_assert(endptr == str);

     /* Fractional values require scale larger than bytes */
-    /* FIXME endptr shouldn't move on -EINVAL */
     str = "1.1B";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert(endptr == str + 4);
+    g_assert(endptr == str);

-    /* FIXME endptr shouldn't move on -EINVAL */
     str = "1.1";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert(endptr == str + 3);
+    g_assert(endptr == str);

-    /* FIXME we should reject floating point exponents */
+    /* No floating point exponents */
     str = "1.5e1k";
     err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 15360);
-    g_assert(endptr == str + 6);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);

-    /* FIXME we should reject floating point exponents */
     str = "1.5E+0k";
     err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 1536);
-    g_assert(endptr == str + 7);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);

-    /* FIXME we should reject hex fractions */
+    /* No hex fractions */
     str = "0x1.8k";
     err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 1536);
-    g_assert(endptr == str + 6);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);

-    /* FIXME we reject all other attempts at negative, why not -0 */
+    /* No negative values */
     str = "-0";
     err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0);
-    g_assert(endptr == str + 2);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = "-1";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
 }

 static void test_qemu_strtosz_trailing(void)
@@ -2263,22 +2260,7 @@ static void test_qemu_strtosz_erange(void)
     int err;
     uint64_t res = 0xbaadf00d;

-    str = "-1";
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert(endptr == str + 2);
-
-    str = "18446744073709550592"; /* 0xfffffffffffffc00 FIXME accept this */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert(endptr == str + 20);
-
-    str = "18446744073709551615"; /* 2^64-1 FIXME accept this */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert(endptr == str + 20);
-
-    str = "18446744073709551616"; /* 2^64 */
+    str = "18446744073709551616"; /* 2^64; see strtosz_simple for 2^64-1 */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -ERANGE);
     g_assert(endptr == str + 20);
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index ee927fe4e427..e20c07cf3ea5 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -445,9 +445,9 @@ static void test_keyval_visit_size(void)
     visit_end_struct(v, NULL);
     visit_free(v);

-    /* Note: precision is 53 bits since we're parsing with strtod() */
+    /* Note: full 64 bits of precision */

-    /* Around limit of precision: 2^53-1, 2^53, 2^53+1 */
+    /* Around double limit of precision: 2^53-1, 2^53, 2^53+1 */
     qdict = keyval_parse("sz1=9007199254740991,"
                          "sz2=9007199254740992,"
                          "sz3=9007199254740993",
@@ -460,22 +460,25 @@ static void test_keyval_visit_size(void)
     visit_type_size(v, "sz2", &sz, &error_abort);
     g_assert_cmphex(sz, ==, 0x20000000000000);
     visit_type_size(v, "sz3", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0x20000000000000);
+    g_assert_cmphex(sz, ==, 0x20000000000001);
     visit_check_struct(v, &error_abort);
     visit_end_struct(v, NULL);
     visit_free(v);

-    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
-    qdict = keyval_parse("sz1=9223372036854774784," /* 7ffffffffffffc00 */
-                         "sz2=9223372036854775295", /* 7ffffffffffffdff */
+    /* Close to signed integer limit 2^63 */
+    qdict = keyval_parse("sz1=9223372036854775807," /* 7fffffffffffffff */
+                         "sz2=9223372036854775808," /* 8000000000000000 */
+                         "sz3=9223372036854775809", /* 8000000000000001 */
                          NULL, 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_size(v, "sz1", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0x7ffffffffffffc00);
+    g_assert_cmphex(sz, ==, 0x7fffffffffffffff);
     visit_type_size(v, "sz2", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0x7ffffffffffffc00);
+    g_assert_cmphex(sz, ==, 0x8000000000000000);
+    visit_type_size(v, "sz3", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x8000000000000001);
     visit_check_struct(v, &error_abort);
     visit_end_struct(v, NULL);
     visit_free(v);
@@ -490,14 +493,26 @@ static void test_keyval_visit_size(void)
     visit_type_size(v, "sz1", &sz, &error_abort);
     g_assert_cmphex(sz, ==, 0xfffffffffffff800);
     visit_type_size(v, "sz2", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0xfffffffffffff800);
+    g_assert_cmphex(sz, ==, 0xfffffffffffffbff);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Actual limit 2^64-1*/
+    qdict = keyval_parse("sz1=18446744073709551615", /* ffffffffffffffff */
+                         NULL, 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_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0xffffffffffffffff);
     visit_check_struct(v, &error_abort);
     visit_end_struct(v, NULL);
     visit_free(v);

     /* Beyond limits */
     qdict = keyval_parse("sz1=-1,"
-                         "sz2=18446744073709550592", /* fffffffffffffc00 */
+                         "sz2=18446744073709551616", /* 2^64 */
                          NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 8bbb17b1c726..6568e31a7229 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -654,9 +654,9 @@ static void test_opts_parse_size(void)
     g_assert_cmpuint(opts_count(opts), ==, 1);
     g_assert_cmpuint(qemu_opt_get_size(opts, "size1", 1), ==, 0);

-    /* Note: precision is 53 bits since we're parsing with strtod() */
+    /* Note: full 64 bits of precision */

-    /* Around limit of precision: 2^53-1, 2^53, 2^54 */
+    /* Around double limit of precision: 2^53-1, 2^53, 2^53+1 */
     opts = qemu_opts_parse(&opts_list_02,
                            "size1=9007199254740991,"
                            "size2=9007199254740992,"
@@ -668,18 +668,21 @@ static void test_opts_parse_size(void)
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
                      ==, 0x20000000000000);
     g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1),
-                     ==, 0x20000000000000);
+                     ==, 0x20000000000001);

-    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
+    /* Close to signed int limit: 2^63-1, 2^63, 2^63+1 */
     opts = qemu_opts_parse(&opts_list_02,
-                           "size1=9223372036854774784," /* 7ffffffffffffc00 */
-                           "size2=9223372036854775295", /* 7ffffffffffffdff */
+                           "size1=9223372036854775807," /* 7fffffffffffffff */
+                           "size2=9223372036854775808," /* 8000000000000000 */
+                           "size3=9223372036854775809", /* 8000000000000001 */
                            false, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 2);
+    g_assert_cmpuint(opts_count(opts), ==, 3);
     g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
-                     ==, 0x7ffffffffffffc00);
+                     ==, 0x7fffffffffffffff);
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
-                     ==, 0x7ffffffffffffc00);
+                     ==, 0x8000000000000000);
+    g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1),
+                     ==, 0x8000000000000001);

     /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
     opts = qemu_opts_parse(&opts_list_02,
@@ -690,14 +693,22 @@ static void test_opts_parse_size(void)
     g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
                      ==, 0xfffffffffffff800);
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
-                     ==, 0xfffffffffffff800);
+                     ==, 0xfffffffffffffbff);
+
+    /* Actual limit, 2^64-1 */
+    opts = qemu_opts_parse(&opts_list_02,
+                           "size1=18446744073709551615", /* ffffffffffffffff */
+                           false, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 1);
+    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
+                     ==, 0xffffffffffffffff);

     /* Beyond limits */
     opts = qemu_opts_parse(&opts_list_02, "size1=-1", false, &err);
     error_free_or_abort(&err);
     g_assert(!opts);
     opts = qemu_opts_parse(&opts_list_02,
-                           "size1=18446744073709550592", /* fffffffffffffc00 */
+                           "size1=18446744073709551616", /* 2^64 */
                            false, &err);
     error_free_or_abort(&err);
     g_assert(!opts);
diff --git a/util/cutils.c b/util/cutils.c
index 0b5073b33012..22d27b0fd21b 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -241,52 +241,100 @@ static int64_t suffix_mul(char suffix, int64_t unit)
 }

 /*
- * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
- * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
- * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
- * other error.
+ * Convert size string to bytes.
+ *
+ * The size parsing supports the following syntaxes
+ * - 12345 - decimal, scale determined by @default_suffix and @unit
+ * - 12345{bBkKmMgGtTpPeE} - decimal, scale determined by suffix and @unit
+ * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, and
+ *   fractional portion is truncated to byte
+ * - 0x7fEE - hexadecimal, unit determined by @default_suffix
+ *
+ * The following are intentionally not supported
+ * - octal, such as 08
+ * - fractional hex, such as 0x1.8
+ * - floating point exponents, such as 1e3
+ *
+ * The end pointer will be returned in *end, if not NULL.  If there is
+ * no fraction, the input can be decimal or hexadecimal; if there is a
+ * fraction, then the input must be decimal and there must be a suffix
+ * (possibly by @default_suffix) larger than Byte, and the fractional
+ * portion may suffer from precision loss or rounding.  The input must
+ * be positive.
+ *
+ * Return -ERANGE on overflow (with *@end advanced), and -EINVAL on
+ * other error (with *@end left unchanged).
  */
 static int do_strtosz(const char *nptr, const char **end,
                       const char default_suffix, int64_t unit,
                       uint64_t *result)
 {
     int retval;
-    const char *endptr;
+    const char *endptr, *f;
     unsigned char c;
-    int mul_required = 0;
-    double val, mul, integral, fraction;
+    bool mul_required = false;
+    uint64_t val;
+    int64_t mul;
+    double fraction = 0.0;

-    retval = qemu_strtod_finite(nptr, &endptr, &val);
+    /* Parse integral portion as decimal. */
+    retval = qemu_strtou64(nptr, &endptr, 10, &val);
     if (retval) {
         goto out;
     }
-    fraction = modf(val, &integral);
-    if (fraction != 0) {
-        mul_required = 1;
+    if (memchr(nptr, '-', endptr - nptr) != NULL) {
+        endptr = nptr;
+        retval = -EINVAL;
+        goto out;
+    }
+    if (val == 0 && (*endptr == 'x' || *endptr == 'X')) {
+        /* Input looks like hex, reparse, and insist on no fraction. */
+        retval = qemu_strtou64(nptr, &endptr, 16, &val);
+        if (retval) {
+            goto out;
+        }
+        if (*endptr == '.') {
+            endptr = nptr;
+            retval = -EINVAL;
+            goto out;
+        }
+    } else if (*endptr == '.') {
+        /*
+         * Input looks like a fraction.  Make sure even 1.k works
+         * without fractional digits.  If we see an exponent, treat
+         * the entire input as invalid instead.
+         */
+        f = endptr;
+        retval = qemu_strtod_finite(f, &endptr, &fraction);
+        if (retval) {
+            fraction = 0.0;
+            endptr++;
+        } else if (memchr(f, 'e', endptr - f) || memchr(f, 'E', endptr - f)) {
+            endptr = nptr;
+            retval = -EINVAL;
+            goto out;
+        } else if (fraction != 0) {
+            mul_required = true;
+        }
     }
     c = *endptr;
     mul = suffix_mul(c, unit);
-    if (mul >= 0) {
+    if (mul > 0) {
         endptr++;
     } else {
         mul = suffix_mul(default_suffix, unit);
-        assert(mul >= 0);
+        assert(mul > 0);
     }
     if (mul == 1 && mul_required) {
+        endptr = nptr;
         retval = -EINVAL;
         goto out;
     }
-    /*
-     * Values near UINT64_MAX overflow to 2**64 when converting to double
-     * precision.  Compare against the maximum representable double precision
-     * value below 2**64, computed as "the next value after 2**64 (0x1p64) in
-     * the direction of 0".
-     */
-    if ((val * mul > nextafter(0x1p64, 0)) || val < 0) {
+    if (val > (UINT64_MAX - ((uint64_t) (fraction * mul))) / mul) {
         retval = -ERANGE;
         goto out;
     }
-    *result = val * mul;
+    *result = val * mul + (uint64_t) (fraction * mul);
     retval = 0;

 out:
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index b1d8fd9107ef..01f7b1f2408b 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -92,16 +92,22 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off comp
 == 3. Invalid sizes ==

 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
-qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.

 qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2
-qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size'
+qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64
+Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
+and exabytes, respectively.

 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k
-qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.

 qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2
-qemu-img: TEST_DIR/t.qcow2: Value '-1k' is out of range for parameter 'size'
+qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64
+Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
+and exabytes, respectively.

 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
 qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index fe193fd5f4ff..0d51fe401ecb 100644
--- a/tests/qemu-iotests/178.out.qcow2
+++ b/tests/qemu-iotests/178.out.qcow2
@@ -13,7 +13,8 @@ qemu-img: Invalid option list: ,
 qemu-img: Invalid parameter 'snapshot.foo'
 qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar'
 qemu-img: --output must be used with human or json as argument.
-qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 qemu-img: Unknown file format 'foo'

 == Size calculation for a new file (human) ==
diff --git a/tests/qemu-iotests/178.out.raw b/tests/qemu-iotests/178.out.raw
index 445e460fad99..116241ddef2f 100644
--- a/tests/qemu-iotests/178.out.raw
+++ b/tests/qemu-iotests/178.out.raw
@@ -13,7 +13,8 @@ qemu-img: Invalid option list: ,
 qemu-img: Invalid parameter 'snapshot.foo'
 qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar'
 qemu-img: --output must be used with human or json as argument.
-qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 qemu-img: Unknown file format 'foo'

 == Size calculation for a new file (human) ==
-- 
2.30.1

Re: [PATCH v2 2/4] utils: Improve qemu_strtosz() to have 64 bits of precision

[Daniel P. Berrangé]

On Thu, Feb 11, 2021 at 02:44:36PM -0600, Eric Blake wrote:
> We have multiple clients of qemu_strtosz (qemu-io, the opts visitor,
> the keyval visitor), and it gets annoying that edge-case testing is
> impacted by implicit rounding to 53 bits of precision due to parsing
> with strtod().  As an example posted by Rich Jones:
>  $ nbdkit memory $(( 2**63 - 2**30 )) --run \
>    'build/qemu-io -f raw "$uri" -c "w -P 3 $(( 2**63 - 2**30 - 512 )) 512" '
>  write failed: Input/output error
> 
> because 9223372035781033472 got rounded to 0x7fffffffc0000000 which is
> out of bounds.
> 
> It is also worth noting that our existing parser, by virtue of using
> strtod(), accepts decimal AND hex numbers, even though test-cutils
> previously lacked any coverage of the latter until the previous patch.
> We do have existing clients that expect a hex parse to work (for
> example, iotest 33 using qemu-io -c "write -P 0xa 0x200 0x400"), but
> strtod() parses "08" as 8 rather than as an invalid octal number, so
> we know there are no clients that depend on octal.  Our use of
> strtod() also means that "0x1.8k" would actually parse as 1536 (the
> fraction is 8/16), rather than 1843 (if the fraction were 8/10); but
> as this was not covered in the testsuite, I have no qualms forbidding
> hex fractions as invalid, so this patch declares that the use of
> fractions is only supported with decimal input, and enhances the
> testsuite to document that.
> 
> Our previous use of strtod() meant that -1 parsed as a negative; now
> that we parse with strtoull(), negative values can wrap around modulo
> 2^64, so we have to explicitly check whether the user passed in a '-';
> and make it consistent to also reject '-0'.  This has the minor effect
> of treating negative values as EINVAL (with no change to endptr)
> rather than ERANGE (with endptr advanced to what was parsed), visible
> in the updated iotest output.
> 
> We also had no testsuite coverage of "1.1e0k", which happened to parse
> under strtod() but is unlikely to occur in practice; as long as we are
> making things more robust, it is easy enough to reject the use of
> exponents in a strtod parse.
> 
> The fix is done by breaking the parse into an integer prefix (no loss
> in precision), rejecting negative values (since we can no longer rely
> on strtod() to do that), determining if a decimal or hexadecimal parse
> was intended (with the new restriction that a fractional hex parse is
> not allowed), and where appropriate, using a floating point fractional
> parse (where we also scan to reject use of exponents in the fraction).
> The bulk of the patch is then updates to the testsuite to match our
> new precision, as well as adding new cases we reject (whether they
> were rejected or inadvertently accepted before).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> 
> Note that this approach differs from what has been attempted in the
> past; see the thread starting at
> https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00852.html
> That approach tried to parse both as strtoull and strtod and take
> whichever was longer, but that was harder to document.
> ---
>  tests/test-cutils.c              | 74 ++++++++++----------------
>  tests/test-keyval.c              | 35 +++++++++----
>  tests/test-qemu-opts.c           | 33 ++++++++----
>  util/cutils.c                    | 90 ++++++++++++++++++++++++--------
>  tests/qemu-iotests/049.out       | 14 +++--
>  tests/qemu-iotests/178.out.qcow2 |  3 +-
>  tests/qemu-iotests/178.out.raw   |  3 +-
>  7 files changed, 158 insertions(+), 94 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes

[Eric Blake]

Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
that is ambiguous for bytes, as well as a less-frequently-used 'E'
suffix for extremely large exibytes.  In practice, people using hex
inputs are specifying values in bytes (and would have written
0x2000000, or possibly relied on default_suffix in the case of
qemu_strtosz_MiB), and the use of scaling suffixes makes the most
sense for inputs in decimal (where the user would write 32M).  But
rather than outright dropping support for hex-with-suffix, let's
follow our deprecation policy.  Sadly, since qemu_strtosz() does not
have an Err** parameter, and plumbing that in would be a much larger
task, we instead go with just directly emitting the deprecation
warning to stderr.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/system/deprecated.rst |  8 ++++++++
 util/cutils.c              | 10 +++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e07..113c2e933f1b 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,14 @@ library enabled as a cryptography provider.
 Neither the ``nettle`` library, or the built-in cryptography provider are
 supported on FIPS enabled hosts.

+hexadecimal sizes with scaling multipliers (since 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Input parameters that take a size value should only use a size suffix
+(such as 'k' or 'M') when the base is written in decimal, and not when
+the value is hexadecimal.  That is, '0x20M' is deprecated, and should
+be written either as '32M' or as '0x2000000'.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------

diff --git a/util/cutils.c b/util/cutils.c
index 22d27b0fd21b..6a8a175e0d71 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit)
  *   fractional portion is truncated to byte
  * - 0x7fEE - hexadecimal, unit determined by @default_suffix
  *
+ * The following cause a deprecation warning, and may be removed in the future
+ * - 0xabc{kKmMgGtTpP} - hex with scaling suffix
+ *
  * The following are intentionally not supported
  * - octal, such as 08
  * - fractional hex, such as 0x1.8
@@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end,
     int retval;
     const char *endptr, *f;
     unsigned char c;
-    bool mul_required = false;
+    bool mul_required = false, hex = false;
     uint64_t val;
     int64_t mul;
     double fraction = 0.0;
@@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end,
             retval = -EINVAL;
             goto out;
         }
+        hex = true;
     } else if (*endptr == '.') {
         /*
          * Input looks like a fraction.  Make sure even 1.k works
@@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end,
     c = *endptr;
     mul = suffix_mul(c, unit);
     if (mul > 0) {
+        if (hex) {
+            warn_report("Using a multiplier suffix on hex numbers "
+                        "is deprecated: %s", nptr);
+        }
         endptr++;
     } else {
         mul = suffix_mul(default_suffix, unit);
-- 
2.30.1

Re: [PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes

[Philippe Mathieu-Daudé]

On 2/11/21 9:44 PM, Eric Blake wrote:
> Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
> that is ambiguous for bytes, as well as a less-frequently-used 'E'
> suffix for extremely large exibytes.  In practice, people using hex
> inputs are specifying values in bytes (and would have written
> 0x2000000, or possibly relied on default_suffix in the case of
> qemu_strtosz_MiB), and the use of scaling suffixes makes the most
> sense for inputs in decimal (where the user would write 32M).  But
> rather than outright dropping support for hex-with-suffix, let's
> follow our deprecation policy.  Sadly, since qemu_strtosz() does not
> have an Err** parameter, and plumbing that in would be a much larger
> task, we instead go with just directly emitting the deprecation
> warning to stderr.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/system/deprecated.rst |  8 ++++++++
>  util/cutils.c              | 10 +++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes

[Daniel P. Berrangé]

On Thu, Feb 11, 2021 at 02:44:37PM -0600, Eric Blake wrote:
> Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
> that is ambiguous for bytes, as well as a less-frequently-used 'E'
> suffix for extremely large exibytes.  In practice, people using hex
> inputs are specifying values in bytes (and would have written
> 0x2000000, or possibly relied on default_suffix in the case of
> qemu_strtosz_MiB), and the use of scaling suffixes makes the most
> sense for inputs in decimal (where the user would write 32M).  But
> rather than outright dropping support for hex-with-suffix, let's
> follow our deprecation policy.  Sadly, since qemu_strtosz() does not
> have an Err** parameter, and plumbing that in would be a much larger
> task, we instead go with just directly emitting the deprecation
> warning to stderr.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/system/deprecated.rst |  8 ++++++++
>  util/cutils.c              | 10 +++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes

[Eric Blake]

The value '1.1k' is inexact; 1126.4 bytes is not possible, so we
happen to truncate it to 1126.  Our use of fractional sizes is
intended for convenience, but when a user specifies a fraction that is
not a clean translation to binary, truncating/rounding behind their
backs can cause confusion.  Better is to deprecate inexact values,
which still leaves '1.5k' as valid, but alerts the user to spell out
their values as a precise byte number in cases where they are
currently being rounded.

Note that values like '0.1G' in the testsuite need adjustment as a
result.

Since qemu_strtosz() does not have an Err** parameter, and plumbing
that in would be a much larger task, we instead go with just directly
emitting the deprecation warning to stderr.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

I'm not a fan of this patch, but am proposing it for discussion purposes.
---
 docs/system/deprecated.rst | 9 +++++++++
 tests/test-cutils.c        | 6 +++---
 tests/test-keyval.c        | 4 ++--
 tests/test-qemu-opts.c     | 4 ++--
 util/cutils.c              | 9 +++++++--
 5 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 113c2e933f1b..2c9cb849eec5 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix
 the value is hexadecimal.  That is, '0x20M' is deprecated, and should
 be written either as '32M' or as '0x2000000'.

+inexact sizes via scaled fractions (since 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''
+
+Input parameters that take a size value should only use a fractional
+size (such as '1.5M') that will result in an exact byte value.  The
+use of inexact values (such as '1.1M') that require truncation or
+rounding is deprecated, and you should instead consider writing your
+unusual size in bytes (here, '1153433' or '1153434' as desired).
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index bad3a6099389..c6c33866277b 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -2124,11 +2124,11 @@ static void test_qemu_strtosz_float(void)
     g_assert_cmpint(res, ==, 1024);
     g_assert(endptr == str + 3);

-    /* For convenience, we permit values that are not byte-exact */
-    str = "12.345M";
+    /* Fractional values should still be byte-exact */
+    str = "12.125M";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));
+    g_assert_cmpint(res, ==, (uint64_t) (12.125 * MiB));
     g_assert(endptr == str + 7);
 }

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index e20c07cf3ea5..7a45c22942e6 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -525,7 +525,7 @@ static void test_keyval_visit_size(void)
     visit_free(v);

     /* Suffixes */
-    qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
+    qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.125G,sz5=16777215T",
                          NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
@@ -537,7 +537,7 @@ static void test_keyval_visit_size(void)
     visit_type_size(v, "sz3", &sz, &error_abort);
     g_assert_cmphex(sz, ==, 2 * MiB);
     visit_type_size(v, "sz4", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, GiB / 10);
+    g_assert_cmphex(sz, ==, GiB / 8);
     visit_type_size(v, "sz5", &sz, &error_abort);
     g_assert_cmphex(sz, ==, 16777215ULL * TiB);
     visit_check_struct(v, &error_abort);
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 6568e31a7229..549e994938fe 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -720,10 +720,10 @@ static void test_opts_parse_size(void)
     g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, 8);
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 1536);
     g_assert_cmphex(qemu_opt_get_size(opts, "size3", 0), ==, 2 * MiB);
-    opts = qemu_opts_parse(&opts_list_02, "size1=0.1G,size2=16777215T",
+    opts = qemu_opts_parse(&opts_list_02, "size1=0.125G,size2=16777215T",
                            false, &error_abort);
     g_assert_cmpuint(opts_count(opts), ==, 2);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 10);
+    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 8);
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215ULL * TiB);

     /* Beyond limit with suffix */
diff --git a/util/cutils.c b/util/cutils.c
index 6a8a175e0d71..1154b9de131a 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -246,12 +246,13 @@ static int64_t suffix_mul(char suffix, int64_t unit)
  * The size parsing supports the following syntaxes
  * - 12345 - decimal, scale determined by @default_suffix and @unit
  * - 12345{bBkKmMgGtTpPeE} - decimal, scale determined by suffix and @unit
- * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, and
- *   fractional portion is truncated to byte
+ * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, if
+ *   fractional portion is exact
  * - 0x7fEE - hexadecimal, unit determined by @default_suffix
  *
  * The following cause a deprecation warning, and may be removed in the future
  * - 0xabc{kKmMgGtTpP} - hex with scaling suffix
+ * - 12345.678{kKmMgGtTpPeE} - decimal with inexact fraction that caused truncation
  *
  * The following are intentionally not supported
  * - octal, such as 08
@@ -342,6 +343,10 @@ static int do_strtosz(const char *nptr, const char **end,
         retval = -ERANGE;
         goto out;
     }
+    if (mul_required && fraction * mul != (uint64_t) (fraction * mul)) {
+        warn_report("Using a fractional size that is not an exact byte "
+                    "multiple is deprecated: %s", nptr);
+    }
     *result = val * mul + (uint64_t) (fraction * mul);
     retval = 0;

-- 
2.30.1

Re: [PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes

[Daniel P. Berrangé]

On Thu, Feb 11, 2021 at 02:44:38PM -0600, Eric Blake wrote:
> The value '1.1k' is inexact; 1126.4 bytes is not possible, so we
> happen to truncate it to 1126.  Our use of fractional sizes is
> intended for convenience, but when a user specifies a fraction that is
> not a clean translation to binary, truncating/rounding behind their
> backs can cause confusion.  Better is to deprecate inexact values,
> which still leaves '1.5k' as valid, but alerts the user to spell out
> their values as a precise byte number in cases where they are
> currently being rounded.
> 
> Note that values like '0.1G' in the testsuite need adjustment as a
> result.
> 
> Since qemu_strtosz() does not have an Err** parameter, and plumbing
> that in would be a much larger task, we instead go with just directly
> emitting the deprecation warning to stderr.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> 
> I'm not a fan of this patch, but am proposing it for discussion purposes.

Likewise. I'm *not* in favour of this patch.

Allowing some fractions but not other fractions forces the potential
user to figure out what the exact fraction is up front, at which point
they've lost the benefit of using fractions. If users actually care
about byte exact values then they already have the option to specify
those exactly. If they've instead chosen to use fractions then they
have implicitly decided they're ok with the potentially in-exact
answer.

IMHO the only question is whethe we should truncate or round, and
I dont really have a preference - either is fine as long as we
are intentionally picking one and documenting it.

> ---
>  docs/system/deprecated.rst | 9 +++++++++
>  tests/test-cutils.c        | 6 +++---
>  tests/test-keyval.c        | 4 ++--
>  tests/test-qemu-opts.c     | 4 ++--
>  util/cutils.c              | 9 +++++++--
>  5 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 113c2e933f1b..2c9cb849eec5 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix
>  the value is hexadecimal.  That is, '0x20M' is deprecated, and should
>  be written either as '32M' or as '0x2000000'.
> 
> +inexact sizes via scaled fractions (since 6.0)
> +''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Input parameters that take a size value should only use a fractional
> +size (such as '1.5M') that will result in an exact byte value.  The
> +use of inexact values (such as '1.1M') that require truncation or
> +rounding is deprecated, and you should instead consider writing your
> +unusual size in bytes (here, '1153433' or '1153434' as desired).


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes

[Eric Blake]

On 2/23/21 11:20 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 11, 2021 at 02:44:38PM -0600, Eric Blake wrote:
>> The value '1.1k' is inexact; 1126.4 bytes is not possible, so we
>> happen to truncate it to 1126.  Our use of fractional sizes is
>> intended for convenience, but when a user specifies a fraction that is
>> not a clean translation to binary, truncating/rounding behind their
>> backs can cause confusion.  Better is to deprecate inexact values,
>> which still leaves '1.5k' as valid, but alerts the user to spell out
>> their values as a precise byte number in cases where they are
>> currently being rounded.
>>
>> Note that values like '0.1G' in the testsuite need adjustment as a
>> result.
>>
>> Since qemu_strtosz() does not have an Err** parameter, and plumbing
>> that in would be a much larger task, we instead go with just directly
>> emitting the deprecation warning to stderr.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>>
>> I'm not a fan of this patch, but am proposing it for discussion purposes.
> 
> Likewise. I'm *not* in favour of this patch.

Glad we're in agreement.  Consider this one dropped, and I will queue
1-3 through my NBD tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/4] improve do_strtosz precision

[Eric Blake]

On 2/11/21 2:44 PM, Eric Blake wrote:
> Parsing sizes with only 53 bits of precision is surprising; it's time
> to fix it to use a full 64 bits of precision.
> 
> v1 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01800.html
> 
> Since then:
> - split testsuite improvements from code changes [Vladimir]
> - more tests for more corner cases [Vladimir, Rich, Dan]
> - fix handling of '123-45' when endptr is non-NULL [Vladimir]
> - fix handling of '1.k'
> - actually enable deprecation of '0x1k' [Vladimir]
> - include missing deprecation text for rounded fractions
> - improved commit messages
> 
> I'm still not sure I like patch 4, but it's at least worth considering.

Ping. I've also just realized that this series will fix:
https://bugzilla.redhat.com/show_bug.cgi?id=1909185
"The error message of "qemu-img convert -r" should advertise the correct
maximum number"

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org