QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
@ 2019-12-05  2:14 Tao Xu
  2019-12-05 15:29 ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Tao Xu @ 2019-12-05  2:14 UTC (permalink / raw)
  To: armbru, mdroth, ehabkost; +Cc: Tao Xu, qemu-devel

Parse input string both as a double and as a uint64_t, then use the
method which consumes more characters. Update the related test cases.

Signed-off-by: Tao Xu <tao3.xu@intel.com>
---
 tests/test-cutils.c    | 37 ++++-----------------
 tests/test-keyval.c    | 47 ++++-----------------------
 tests/test-qemu-opts.c | 39 ++++------------------
 util/cutils.c          | 74 ++++++++++++++++++++++++++++++------------
 4 files changed, 73 insertions(+), 124 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 1aa8351520..4a7030c611 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1970,40 +1970,25 @@ 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);
-    g_assert_cmpint(res, ==, 0x1fffffffffffff);
-    g_assert(endptr == str + 16);
-
-    str = "9007199254740992"; /* 2^53 */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0x20000000000000);
-    g_assert(endptr == str + 16);
+    /* Note: precision is 64 bits (UINT64_MAX) */
 
     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) */
+    str = "18446744073709550591"; /* 0xfffffffffffffbff */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0xfffffffffffff800);
+    g_assert_cmpint(res, ==, 0xfffffffffffffbff);
     g_assert(endptr == str + 20);
 
-    str = "18446744073709550591"; /* 0xfffffffffffffbff */
+    str = "18446744073709551615"; /* 2^64-1 (UINT64_MAX) */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */
+    g_assert_cmpint(res, ==, 0xffffffffffffffff);
     g_assert(endptr == str + 20);
-
-    /* 0x7ffffffffffffe00..0x7fffffffffffffff get rounded to
-     * 0x8000000000000000, thus -ERANGE; see test_qemu_strtosz_erange() */
 }
 
 static void test_qemu_strtosz_units(void)
@@ -2145,16 +2130,6 @@ static void test_qemu_strtosz_erange(void)
     g_assert_cmpint(err, ==, -ERANGE);
     g_assert(endptr == str + 2);
 
-    str = "18446744073709550592"; /* 0xfffffffffffffc00 */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert(endptr == str + 20);
-
-    str = "18446744073709551615"; /* 2^64-1 */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert(endptr == str + 20);
-
     str = "18446744073709551616"; /* 2^64 */
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -ERANGE);
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 09b0ae3c68..fad941fcb8 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -383,59 +383,26 @@ 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: precision is 64 bits (UINT64_MAX) */
 
-    /* Around limit of precision: 2^53-1, 2^53, 2^53+1 */
-    qdict = keyval_parse("sz1=9007199254740991,"
-                         "sz2=9007199254740992,"
-                         "sz3=9007199254740993",
+    /* Around limit of precision: UINT64_MAX - 1, UINT64_MAX */
+    qdict = keyval_parse("sz1=18446744073709551614,"
+                         "sz2=18446744073709551615",
                          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, ==, 0x1fffffffffffff);
+    g_assert_cmphex(sz, ==, 0xfffffffffffffffe);
     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);
-    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 */
-                         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);
-    visit_type_size(v, "sz2", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0x7ffffffffffffc00);
-    visit_check_struct(v, &error_abort);
-    visit_end_struct(v, NULL);
-    visit_free(v);
-
-    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
-    qdict = keyval_parse("sz1=18446744073709549568," /* fffffffffffff800 */
-                         "sz2=18446744073709550591", /* fffffffffffffbff */
-                         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, ==, 0xfffffffffffff800);
-    visit_type_size(v, "sz2", &sz, &error_abort);
-    g_assert_cmphex(sz, ==, 0xfffffffffffff800);
+    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, &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 ef96e84aed..3a8b8c0168 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -650,50 +650,25 @@ 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: precision is 64 bits (UINT64_MAX) */
 
-    /* Around limit of precision: 2^53-1, 2^53, 2^54 */
+    /* Around limit of precision: UINT64_MAX - 1, UINT64_MAX */
     opts = qemu_opts_parse(&opts_list_02,
-                           "size1=9007199254740991,"
-                           "size2=9007199254740992,"
-                           "size3=9007199254740993",
-                           false, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 3);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
-                     ==, 0x1fffffffffffff);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
-                     ==, 0x20000000000000);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1),
-                     ==, 0x20000000000000);
-
-    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
-    opts = qemu_opts_parse(&opts_list_02,
-                           "size1=9223372036854774784," /* 7ffffffffffffc00 */
-                           "size2=9223372036854775295", /* 7ffffffffffffdff */
-                           false, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 2);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
-                     ==, 0x7ffffffffffffc00);
-    g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
-                     ==, 0x7ffffffffffffc00);
-
-    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
-    opts = qemu_opts_parse(&opts_list_02,
-                           "size1=18446744073709549568," /* fffffffffffff800 */
-                           "size2=18446744073709550591", /* fffffffffffffbff */
+                           "size1=18446744073709551614,"
+                           "size2=18446744073709551615",
                            false, &error_abort);
     g_assert_cmpuint(opts_count(opts), ==, 2);
     g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1),
-                     ==, 0xfffffffffffff800);
+                     ==, 0xfffffffffffffffe);
     g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1),
-                     ==, 0xfffffffffffff800);
+                     ==, 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 77acadc70a..b08058c57c 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -212,24 +212,43 @@ 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;
+    int retval, retd, retu;
+    const char *suffix, *suffixd, *suffixu;
     unsigned char c;
     int mul_required = 0;
-    double val, mul, integral, fraction;
+    bool use_strtod;
+    uint64_t valu;
+    double vald, mul, integral, fraction;
+
+    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
+    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
+    use_strtod = strlen(suffixd) < strlen(suffixu);
+
+    /*
+     * Parse @nptr both as a double and as a uint64_t, then use the method
+     * which consumes more characters.
+     */
+    if (use_strtod) {
+        suffix = suffixd;
+        retval = retd;
+    } else {
+        suffix = suffixu;
+        retval = retu;
+    }
 
-    retval = qemu_strtod_finite(nptr, &endptr, &val);
     if (retval) {
         goto out;
     }
-    fraction = modf(val, &integral);
-    if (fraction != 0) {
-        mul_required = 1;
+    if (use_strtod) {
+        fraction = modf(vald, &integral);
+        if (fraction != 0) {
+            mul_required = 1;
+        }
     }
-    c = *endptr;
+    c = *suffix;
     mul = suffix_mul(c, unit);
     if (mul >= 0) {
-        endptr++;
+        suffix++;
     } else {
         mul = suffix_mul(default_suffix, unit);
         assert(mul >= 0);
@@ -238,23 +257,36 @@ static int do_strtosz(const char *nptr, const char **end,
         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) {
-        retval = -ERANGE;
-        goto out;
+
+    if (use_strtod) {
+        /*
+         * 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 ((vald * mul > nextafter(0x1p64, 0)) || vald < 0) {
+            retval = -ERANGE;
+            goto out;
+        }
+        *result = vald * mul;
+    } else {
+        /* Reject negative input and overflow output */
+        while (qemu_isspace(*nptr)) {
+            nptr++;
+        }
+        if (*nptr == '-' || UINT64_MAX / (uint64_t) mul < valu) {
+            retval = -ERANGE;
+            goto out;
+        }
+        *result = valu * (uint64_t) mul;
     }
-    *result = val * mul;
     retval = 0;
 
 out:
     if (end) {
-        *end = endptr;
-    } else if (*endptr) {
+        *end = suffix;
+    } else if (*suffix) {
         retval = -EINVAL;
     }
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-05  2:14 [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits Tao Xu
@ 2019-12-05 15:29 ` Markus Armbruster
  2019-12-09  5:38   ` Tao Xu
  2019-12-17 12:04   ` Christophe de Dinechin
  0 siblings, 2 replies; 15+ messages in thread
From: Markus Armbruster @ 2019-12-05 15:29 UTC (permalink / raw)
  To: Tao Xu; +Cc: qemu-devel, mdroth, ehabkost

Tao Xu <tao3.xu@intel.com> writes:

> Parse input string both as a double and as a uint64_t, then use the
> method which consumes more characters. Update the related test cases.
>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
[...]
> diff --git a/util/cutils.c b/util/cutils.c
> index 77acadc70a..b08058c57c 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -212,24 +212,43 @@ 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;
> +    int retval, retd, retu;
> +    const char *suffix, *suffixd, *suffixu;
>      unsigned char c;
>      int mul_required = 0;
> -    double val, mul, integral, fraction;
> +    bool use_strtod;
> +    uint64_t valu;
> +    double vald, mul, integral, fraction;

Note for later: @mul is double.

> +
> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
> +    use_strtod = strlen(suffixd) < strlen(suffixu);
> +
> +    /*
> +     * Parse @nptr both as a double and as a uint64_t, then use the method
> +     * which consumes more characters.
> +     */

The comment is in a funny place.  I'd put it right before the
qemu_strtod_finite() line.

> +    if (use_strtod) {
> +        suffix = suffixd;
> +        retval = retd;
> +    } else {
> +        suffix = suffixu;
> +        retval = retu;
> +    }
>  
> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>      if (retval) {
>          goto out;
>      }

This is even more subtle than it looks.

A close reading of the function contracts leads to three cases for each
conversion:

* parse error (including infinity and NaN)

  @retu / @retd is -EINVAL
  @valu / @vald is uninitialized
  @suffixu / @suffixd is @nptr

* range error

  @retu / @retd is -ERANGE
  @valu / @vald is our best approximation of the conversion result
  @suffixu / @suffixd points to the first character not consumed by the
  conversion.

  Sub-cases:

  - uint64_t overflow

    We know the conversion result exceeds UINT64_MAX.

  - double overflow

    we know the conversion result's magnitude exceeds the largest
    representable finite double DBL_MAX.

  - double underflow

    we know the conversion result is close to zero (closer than DBL_MIN,
    the smallest normalized positive double).

* success

  @retu / @retd is 0
  @valu / @vald is the conversion result
  @suffixu / @suffixd points to the first character not consumed by the
  conversion.

This leads to a matrix (parse error, uint64_t overflow, success) x
(parse error, double overflow, double underflow, success).  We need to
check the code does what we want for each element of this matrix, and
document any behavior that's not perfectly obvious.

(success, success): we pick uint64_t if qemu_strtou64() consumed more
characters than qemu_strtod_finite(), else double.  "More" is important
here; when they consume the same characters, we *need* to use the
uint64_t result.  Example: for "18446744073709551615", we need to use
uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
for "18446744073709551616.", we need to use the double.  Good.

(success, parse error) and (parse error, success): we pick the one that
succeeds, because success consumes characters, and failure to parse does
not.  Good.

(parse error, parse error): neither consumes characters, so we pick
uint64_t.  Good.

(parse error, double overflow), (parse error, double underflow) and
(uint64_t overflow, parse error): we pick the range error, because it
consumes characters.  Good.

These are the simple combinations.  The remainder are hairier: (success,
double overflow), (success, double underflow), (uint64_t overflow,
success).  I lack the time to analyze them today.  Must be done before
we take this patch.  Any takers?

> -    fraction = modf(val, &integral);
> -    if (fraction != 0) {
> -        mul_required = 1;
> +    if (use_strtod) {
> +        fraction = modf(vald, &integral);
> +        if (fraction != 0) {
> +            mul_required = 1;
> +        }
>      }

Here, @suffix points to the suffix character, if any.

> -    c = *endptr;
> +    c = *suffix;
>      mul = suffix_mul(c, unit);
>      if (mul >= 0) {
> -        endptr++;
> +        suffix++;

Now @suffix points to the first character not consumed, *not* the
suffix.

Your patch effectively renames @endptr to @suffix.  I think @endptr is
the better name.  Keeping the name also makes the diff smaller and
slightly easier to review.

>      } else {
>          mul = suffix_mul(default_suffix, unit);

suffix_mul() returns int64_t.  The assignment converts it to double.
Fine before the patch, because @mul is the multiplier for a double
value.  No longer true after the patch, see below.

>          assert(mul >= 0);
> @@ -238,23 +257,36 @@ static int do_strtosz(const char *nptr, const char **end,
>          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) {
> -        retval = -ERANGE;
> -        goto out;
> +
> +    if (use_strtod) {
> +        /*
> +         * 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 ((vald * mul > nextafter(0x1p64, 0)) || vald < 0) {
> +            retval = -ERANGE;
> +            goto out;
> +        }
> +        *result = vald * mul;

Here, @mul is a multiplier for double vald.

> +    } else {
> +        /* Reject negative input and overflow output */
> +        while (qemu_isspace(*nptr)) {
> +            nptr++;
> +        }
> +        if (*nptr == '-' || UINT64_MAX / (uint64_t) mul < valu) {
> +            retval = -ERANGE;
> +            goto out;
> +        }
> +        *result = valu * (uint64_t) mul;

Here, @mul is a multiplier for uint64_t valu.

Please change @mul to int64_t to reduce conversions.

>      }
> -    *result = val * mul;
>      retval = 0;
>  
>  out:
>      if (end) {
> -        *end = endptr;
> -    } else if (*endptr) {
> +        *end = suffix;
> +    } else if (*suffix) {
>          retval = -EINVAL;
>      }



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-05 15:29 ` Markus Armbruster
@ 2019-12-09  5:38   ` Tao Xu
  2019-12-17 10:25     ` Markus Armbruster
  2019-12-17 12:04   ` Christophe de Dinechin
  1 sibling, 1 reply; 15+ messages in thread
From: Tao Xu @ 2019-12-09  5:38 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, mdroth, ehabkost



On 12/5/19 11:29 PM, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> Parse input string both as a double and as a uint64_t, then use the
>> method which consumes more characters. Update the related test cases.
>>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
> [...]
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 77acadc70a..b08058c57c 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -212,24 +212,43 @@ 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;
>> +    int retval, retd, retu;
>> +    const char *suffix, *suffixd, *suffixu;
>>       unsigned char c;
>>       int mul_required = 0;
>> -    double val, mul, integral, fraction;
>> +    bool use_strtod;
>> +    uint64_t valu;
>> +    double vald, mul, integral, fraction;
> 
> Note for later: @mul is double.
> 
>> +
>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>> +
>> +    /*
>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>> +     * which consumes more characters.
>> +     */
> 
> The comment is in a funny place.  I'd put it right before the
> qemu_strtod_finite() line.
> 
>> +    if (use_strtod) {
>> +        suffix = suffixd;
>> +        retval = retd;
>> +    } else {
>> +        suffix = suffixu;
>> +        retval = retu;
>> +    }
>>   
>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>       if (retval) {
>>           goto out;
>>       }
> 
> This is even more subtle than it looks.
> 
> A close reading of the function contracts leads to three cases for each
> conversion:
> 
> * parse error (including infinity and NaN)
> 
>    @retu / @retd is -EINVAL
>    @valu / @vald is uninitialized
>    @suffixu / @suffixd is @nptr
> 
> * range error
> 
>    @retu / @retd is -ERANGE
>    @valu / @vald is our best approximation of the conversion result
>    @suffixu / @suffixd points to the first character not consumed by the
>    conversion.
> 
>    Sub-cases:
> 
>    - uint64_t overflow
> 
>      We know the conversion result exceeds UINT64_MAX.
> 
>    - double overflow
> 
>      we know the conversion result's magnitude exceeds the largest
>      representable finite double DBL_MAX.
> 
>    - double underflow
> 
>      we know the conversion result is close to zero (closer than DBL_MIN,
>      the smallest normalized positive double).
> 
> * success
> 
>    @retu / @retd is 0
>    @valu / @vald is the conversion result
>    @suffixu / @suffixd points to the first character not consumed by the
>    conversion.
> 
> This leads to a matrix (parse error, uint64_t overflow, success) x
> (parse error, double overflow, double underflow, success).  We need to
> check the code does what we want for each element of this matrix, and
> document any behavior that's not perfectly obvious.
> 
> (success, success): we pick uint64_t if qemu_strtou64() consumed more
> characters than qemu_strtod_finite(), else double.  "More" is important
> here; when they consume the same characters, we *need* to use the
> uint64_t result.  Example: for "18446744073709551615", we need to use
> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
> for "18446744073709551616.", we need to use the double.  Good.
> 
> (success, parse error) and (parse error, success): we pick the one that
> succeeds, because success consumes characters, and failure to parse does
> not.  Good.
> 
> (parse error, parse error): neither consumes characters, so we pick
> uint64_t.  Good.
> 
> (parse error, double overflow), (parse error, double underflow) and
> (uint64_t overflow, parse error): we pick the range error, because it
> consumes characters.  Good.
> 
> These are the simple combinations.  The remainder are hairier: (success,
> double overflow), (success, double underflow), (uint64_t overflow,
> success).  I lack the time to analyze them today.  Must be done before
> we take this patch.  Any takers?

(success, double overflow), (success, double underflow), pick double 
overflow error, return -ERANGE. Because it consumes characters. Example: 
for "1.79769e+309", qemu_strtou64 consumes "1", and prases as uint64_t; 
but qemu_strtod_finite return -ERANGE and consumes all characters. It is OK.

(uint64_t overflow, success), consume the same characters, use the
uint64_t return -ERANGE. Note that even if qemu_strtod_finite can parse 
these cases such as "18446744073709551617", but the result is uint64_t 
so we also need to return -ERANGE. It is OK.

Thank you for your analysis and suggestion. I will add more test cases 
to cover some of these analysis.
> 
>> -    fraction = modf(val, &integral);
>> -    if (fraction != 0) {
>> -        mul_required = 1;
>> +    if (use_strtod) {
>> +        fraction = modf(vald, &integral);
>> +        if (fraction != 0) {
>> +            mul_required = 1;
>> +        }
>>       }
> 
> Here, @suffix points to the suffix character, if any.
> 
>> -    c = *endptr;
>> +    c = *suffix;
>>       mul = suffix_mul(c, unit);
>>       if (mul >= 0) {
>> -        endptr++;
>> +        suffix++;
> 
> Now @suffix points to the first character not consumed, *not* the
> suffix.
> 
> Your patch effectively renames @endptr to @suffix.  I think @endptr is
> the better name.  Keeping the name also makes the diff smaller and
> slightly easier to review.
> 
>>       } else {
>>           mul = suffix_mul(default_suffix, unit);
> 
> suffix_mul() returns int64_t.  The assignment converts it to double.
> Fine before the patch, because @mul is the multiplier for a double
> value.  No longer true after the patch, see below.
> 
>>           assert(mul >= 0);
>> @@ -238,23 +257,36 @@ static int do_strtosz(const char *nptr, const char **end,
>>           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) {
>> -        retval = -ERANGE;
>> -        goto out;
>> +
>> +    if (use_strtod) {
>> +        /*
>> +         * 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 ((vald * mul > nextafter(0x1p64, 0)) || vald < 0) {
>> +            retval = -ERANGE;
>> +            goto out;
>> +        }
>> +        *result = vald * mul;
> 
> Here, @mul is a multiplier for double vald.
> 
>> +    } else {
>> +        /* Reject negative input and overflow output */
>> +        while (qemu_isspace(*nptr)) {
>> +            nptr++;
>> +        }
>> +        if (*nptr == '-' || UINT64_MAX / (uint64_t) mul < valu) {
>> +            retval = -ERANGE;
>> +            goto out;
>> +        }
>> +        *result = valu * (uint64_t) mul;
> 
> Here, @mul is a multiplier for uint64_t valu.
> 
> Please change @mul to int64_t to reduce conversions.
> 
>>       }
>> -    *result = val * mul;
>>       retval = 0;
>>   
>>   out:
>>       if (end) {
>> -        *end = endptr;
>> -    } else if (*endptr) {
>> +        *end = suffix;
>> +    } else if (*suffix) {
>>           retval = -EINVAL;
>>       }
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-09  5:38   ` Tao Xu
@ 2019-12-17 10:25     ` Markus Armbruster
  2019-12-18  1:33       ` Tao Xu
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-12-17 10:25 UTC (permalink / raw)
  To: Tao Xu; +Cc: qemu-devel, ehabkost, mdroth

Tao Xu <tao3.xu@intel.com> writes:

> On 12/5/19 11:29 PM, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> Parse input string both as a double and as a uint64_t, then use the
>>> method which consumes more characters. Update the related test cases.
>>>
>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>> ---
>> [...]
>>> diff --git a/util/cutils.c b/util/cutils.c
>>> index 77acadc70a..b08058c57c 100644
>>> --- a/util/cutils.c
>>> +++ b/util/cutils.c
>>> @@ -212,24 +212,43 @@ 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;
>>> +    int retval, retd, retu;
>>> +    const char *suffix, *suffixd, *suffixu;
>>>       unsigned char c;
>>>       int mul_required = 0;
>>> -    double val, mul, integral, fraction;
>>> +    bool use_strtod;
>>> +    uint64_t valu;
>>> +    double vald, mul, integral, fraction;
>>
>> Note for later: @mul is double.
>>
>>> +
>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);

Note for later: passing 0 to base accepts octal and hexadecimal
integers.

>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>> +
>>> +    /*
>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>> +     * which consumes more characters.
>>> +     */
>>
>> The comment is in a funny place.  I'd put it right before the
>> qemu_strtod_finite() line.
>>
>>> +    if (use_strtod) {
>>> +        suffix = suffixd;
>>> +        retval = retd;
>>> +    } else {
>>> +        suffix = suffixu;
>>> +        retval = retu;
>>> +    }
>>>   -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>       if (retval) {
>>>           goto out;
>>>       }
>>
>> This is even more subtle than it looks.
>>
>> A close reading of the function contracts leads to three cases for each
>> conversion:
>>
>> * parse error (including infinity and NaN)
>>
>>    @retu / @retd is -EINVAL
>>    @valu / @vald is uninitialized
>>    @suffixu / @suffixd is @nptr
>>
>> * range error
>>
>>    @retu / @retd is -ERANGE
>>    @valu / @vald is our best approximation of the conversion result
>>    @suffixu / @suffixd points to the first character not consumed by the
>>    conversion.
>>
>>    Sub-cases:
>>
>>    - uint64_t overflow
>>
>>      We know the conversion result exceeds UINT64_MAX.
>>
>>    - double overflow
>>
>>      we know the conversion result's magnitude exceeds the largest
>>      representable finite double DBL_MAX.
>>
>>    - double underflow
>>
>>      we know the conversion result is close to zero (closer than DBL_MIN,
>>      the smallest normalized positive double).
>>
>> * success
>>
>>    @retu / @retd is 0
>>    @valu / @vald is the conversion result
>>    @suffixu / @suffixd points to the first character not consumed by the
>>    conversion.
>>
>> This leads to a matrix (parse error, uint64_t overflow, success) x
>> (parse error, double overflow, double underflow, success).  We need to
>> check the code does what we want for each element of this matrix, and
>> document any behavior that's not perfectly obvious.
>>
>> (success, success): we pick uint64_t if qemu_strtou64() consumed more
>> characters than qemu_strtod_finite(), else double.  "More" is important
>> here; when they consume the same characters, we *need* to use the
>> uint64_t result.  Example: for "18446744073709551615", we need to use
>> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
>> for "18446744073709551616.", we need to use the double.  Good.

Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
"0123.", we use 123.0, not 83.

Do we really want to accept octal and hexadecimal integers?

>> (success, parse error) and (parse error, success): we pick the one that
>> succeeds, because success consumes characters, and failure to parse does
>> not.  Good.
>>
>> (parse error, parse error): neither consumes characters, so we pick
>> uint64_t.  Good.
>>
>> (parse error, double overflow), (parse error, double underflow) and
>> (uint64_t overflow, parse error): we pick the range error, because it
>> consumes characters.  Good.
>>
>> These are the simple combinations.  The remainder are hairier: (success,
>> double overflow), (success, double underflow), (uint64_t overflow,
>> success).  I lack the time to analyze them today.  Must be done before
>> we take this patch.  Any takers?
>
> (success, double overflow), (success, double underflow), pick double
> overflow error, return -ERANGE. Because it consumes
> characters. Example: for "1.79769e+309", qemu_strtou64 consumes "1",
> and prases as uint64_t; but qemu_strtod_finite return -ERANGE and
> consumes all characters. It is OK.

The only way to have double overflow when uint64_t succeeds is an
exponent.  Double consumes the characters making up the exponent,
uint64_t does not.  We use double.

The only way to have double underflow is with an exponent or a decimal
point.  Double consumes their characters, uint64_t does not.  We use
double.

Okay.

> (uint64_t overflow, success), consume the same characters, use the
> uint64_t return -ERANGE. Note that even if qemu_strtod_finite can
> parse these cases such as "18446744073709551617", but the result is
> uint64_t so we also need to return -ERANGE. It is OK.

That's just one of two cases, I think.  The other one is when the
overflowing integer is followed by an exponent or decimal point.  We use
double then.  Converting the double to uint64_t overflows, except when a
negative exponent brings the number into range.

Examples: "18446744073709551617" picks uint64_t overflow,
"18446744073709551617.0" picks double success (but converting it to
uint64_t below overflows), and "18446744073709551617e-10" picks double
success (converted to 1844674407 below).

Okay.

> Thank you for your analysis and suggestion. I will add more test cases
> to cover some of these analysis.

Good move.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-05 15:29 ` Markus Armbruster
  2019-12-09  5:38   ` Tao Xu
@ 2019-12-17 12:04   ` Christophe de Dinechin
  2019-12-17 14:08     ` Markus Armbruster
  1 sibling, 1 reply; 15+ messages in thread
From: Christophe de Dinechin @ 2019-12-17 12:04 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Tao Xu, qemu-devel, ehabkost, mdroth



> On 5 Dec 2019, at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> Parse input string both as a double and as a uint64_t, then use the
>> method which consumes more characters. Update the related test cases.
>> 
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
> [...]
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 77acadc70a..b08058c57c 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -212,24 +212,43 @@ 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;
>> +    int retval, retd, retu;
>> +    const char *suffix, *suffixd, *suffixu;
>>     unsigned char c;
>>     int mul_required = 0;
>> -    double val, mul, integral, fraction;
>> +    bool use_strtod;
>> +    uint64_t valu;
>> +    double vald, mul, integral, fraction;
> 
> Note for later: @mul is double.
> 
>> +
>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>> +
>> +    /*
>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>> +     * which consumes more characters.
>> +     */
> 
> The comment is in a funny place.  I'd put it right before the
> qemu_strtod_finite() line.
> 
>> +    if (use_strtod) {
>> +        suffix = suffixd;
>> +        retval = retd;
>> +    } else {
>> +        suffix = suffixu;
>> +        retval = retu;
>> +    }
>> 
>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>     if (retval) {
>>         goto out;
>>     }
> 
> This is even more subtle than it looks.

But why it is even necessary?

The “contract” for the function used to be that it returned rounded values
beyond 2^53, which in itself is curious.

But now it’s a 6-dimensional matrix of hell with NaNs and barfnots, when the
name implies it’s simply doing a text to u64 conversion…

There is certainly a reason, but I’m really curious what it is :-)

> 
> A close reading of the function contracts leads to three cases for each
> conversion:
> 
> * parse error (including infinity and NaN)
> 
>  @retu / @retd is -EINVAL
>  @valu / @vald is uninitialized
>  @suffixu / @suffixd is @nptr
> 
> * range error
> 
>  @retu / @retd is -ERANGE
>  @valu / @vald is our best approximation of the conversion result
>  @suffixu / @suffixd points to the first character not consumed by the
>  conversion.
> 
>  Sub-cases:
> 
>  - uint64_t overflow
> 
>    We know the conversion result exceeds UINT64_MAX.
> 
>  - double overflow
> 
>    we know the conversion result's magnitude exceeds the largest
>    representable finite double DBL_MAX.
> 
>  - double underflow
> 
>    we know the conversion result is close to zero (closer than DBL_MIN,
>    the smallest normalized positive double).
> 
> * success
> 
>  @retu / @retd is 0
>  @valu / @vald is the conversion result
>  @suffixu / @suffixd points to the first character not consumed by the
>  conversion.
> 
> This leads to a matrix (parse error, uint64_t overflow, success) x
> (parse error, double overflow, double underflow, success).  We need to
> check the code does what we want for each element of this matrix, and
> document any behavior that's not perfectly obvious.
> 
> (success, success): we pick uint64_t if qemu_strtou64() consumed more
> characters than qemu_strtod_finite(), else double.  "More" is important
> here; when they consume the same characters, we *need* to use the
> uint64_t result.  Example: for "18446744073709551615", we need to use
> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
> for "18446744073709551616.", we need to use the double.  Good.
> 
> (success, parse error) and (parse error, success): we pick the one that
> succeeds, because success consumes characters, and failure to parse does
> not.  Good.
> 
> (parse error, parse error): neither consumes characters, so we pick
> uint64_t.  Good.
> 
> (parse error, double overflow), (parse error, double underflow) and
> (uint64_t overflow, parse error): we pick the range error, because it
> consumes characters.  Good.
> 
> These are the simple combinations.  The remainder are hairier: (success,
> double overflow), (success, double underflow), (uint64_t overflow,
> success).  I lack the time to analyze them today.  Must be done before
> we take this patch.  Any takers?
> 
>> -    fraction = modf(val, &integral);
>> -    if (fraction != 0) {
>> -        mul_required = 1;
>> +    if (use_strtod) {
>> +        fraction = modf(vald, &integral);
>> +        if (fraction != 0) {
>> +            mul_required = 1;
>> +        }
>>     }
> 
> Here, @suffix points to the suffix character, if any.
> 
>> -    c = *endptr;
>> +    c = *suffix;
>>     mul = suffix_mul(c, unit);
>>     if (mul >= 0) {
>> -        endptr++;
>> +        suffix++;
> 
> Now @suffix points to the first character not consumed, *not* the
> suffix.
> 
> Your patch effectively renames @endptr to @suffix.  I think @endptr is
> the better name.  Keeping the name also makes the diff smaller and
> slightly easier to review.
> 
>>     } else {
>>         mul = suffix_mul(default_suffix, unit);
> 
> suffix_mul() returns int64_t.  The assignment converts it to double.
> Fine before the patch, because @mul is the multiplier for a double
> value.  No longer true after the patch, see below.
> 
>>         assert(mul >= 0);
>> @@ -238,23 +257,36 @@ static int do_strtosz(const char *nptr, const char **end,
>>         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) {
>> -        retval = -ERANGE;
>> -        goto out;
>> +
>> +    if (use_strtod) {
>> +        /*
>> +         * 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 ((vald * mul > nextafter(0x1p64, 0)) || vald < 0) {
>> +            retval = -ERANGE;
>> +            goto out;
>> +        }
>> +        *result = vald * mul;
> 
> Here, @mul is a multiplier for double vald.
> 
>> +    } else {
>> +        /* Reject negative input and overflow output */
>> +        while (qemu_isspace(*nptr)) {
>> +            nptr++;
>> +        }
>> +        if (*nptr == '-' || UINT64_MAX / (uint64_t) mul < valu) {
>> +            retval = -ERANGE;
>> +            goto out;
>> +        }
>> +        *result = valu * (uint64_t) mul;
> 
> Here, @mul is a multiplier for uint64_t valu.
> 
> Please change @mul to int64_t to reduce conversions.
> 
>>     }
>> -    *result = val * mul;
>>     retval = 0;
>> 
>> out:
>>     if (end) {
>> -        *end = endptr;
>> -    } else if (*endptr) {
>> +        *end = suffix;
>> +    } else if (*suffix) {
>>         retval = -EINVAL;
>>     }
> 
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-17 12:04   ` Christophe de Dinechin
@ 2019-12-17 14:08     ` Markus Armbruster
  2019-12-17 14:12       ` Christophe de Dinechin
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-12-17 14:08 UTC (permalink / raw)
  To: Christophe de Dinechin; +Cc: mdroth, Tao Xu, qemu-devel, ehabkost

Christophe de Dinechin <dinechin@redhat.com> writes:

>> On 5 Dec 2019, at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Tao Xu <tao3.xu@intel.com> writes:
>> 
>>> Parse input string both as a double and as a uint64_t, then use the
>>> method which consumes more characters. Update the related test cases.
>>> 
>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>> ---
>> [...]
>>> diff --git a/util/cutils.c b/util/cutils.c
>>> index 77acadc70a..b08058c57c 100644
>>> --- a/util/cutils.c
>>> +++ b/util/cutils.c
>>> @@ -212,24 +212,43 @@ 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;
>>> +    int retval, retd, retu;
>>> +    const char *suffix, *suffixd, *suffixu;
>>>     unsigned char c;
>>>     int mul_required = 0;
>>> -    double val, mul, integral, fraction;
>>> +    bool use_strtod;
>>> +    uint64_t valu;
>>> +    double vald, mul, integral, fraction;
>> 
>> Note for later: @mul is double.
>> 
>>> +
>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>> +
>>> +    /*
>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>> +     * which consumes more characters.
>>> +     */
>> 
>> The comment is in a funny place.  I'd put it right before the
>> qemu_strtod_finite() line.
>> 
>>> +    if (use_strtod) {
>>> +        suffix = suffixd;
>>> +        retval = retd;
>>> +    } else {
>>> +        suffix = suffixu;
>>> +        retval = retu;
>>> +    }
>>> 
>>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>     if (retval) {
>>>         goto out;
>>>     }
>> 
>> This is even more subtle than it looks.
>
> But why it is even necessary?
>
> The “contract” for the function used to be that it returned rounded values
> beyond 2^53, which in itself is curious.
>
> But now it’s a 6-dimensional matrix of hell with NaNs and barfnots, when the
> name implies it’s simply doing a text to u64 conversion…
>
> There is certainly a reason, but I’m really curious what it is :-)

It all goes back to commit 9f9b17a4f0 "Introduce strtosz() library
function to convert a string to a byte count.".  To support "convenient"
usage like "1.5G", it parses the number part with strtod().  This limits
us to 53 bits of precision.  Larger sizes get rounded.

I guess the excuse for this was that when you're dealing with sizes that
large (petabytes!), your least significant bits are zero anyway.

Regardless, the interface is *awful*.  We should've forced the author to
spell it out in all its glory in a proper function contract.  That tends
to cool the enthusiasm for "convenient" syntax amazingly fast.

The awful interface has been confusing people for close to a decade now.

What to do?

Tao Xu's patch tries to make the function do what its users expect,
namely parse a bleepin' 64 bit integer, without breaking any of the
"convenience" syntax.  Turns out that's amazingly subtle.  Are we making
things less confusing or more?



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-17 14:08     ` Markus Armbruster
@ 2019-12-17 14:12       ` Christophe de Dinechin
  2019-12-17 15:01         ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Christophe de Dinechin @ 2019-12-17 14:12 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: mdroth, Tao Xu, qemu-devel, ehabkost



> On 17 Dec 2019, at 15:08, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Christophe de Dinechin <dinechin@redhat.com> writes:
> 
>>> On 5 Dec 2019, at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Tao Xu <tao3.xu@intel.com> writes:
>>> 
>>>> Parse input string both as a double and as a uint64_t, then use the
>>>> method which consumes more characters. Update the related test cases.
>>>> 
>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>> ---
>>> [...]
>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>> index 77acadc70a..b08058c57c 100644
>>>> --- a/util/cutils.c
>>>> +++ b/util/cutils.c
>>>> @@ -212,24 +212,43 @@ 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;
>>>> +    int retval, retd, retu;
>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>    unsigned char c;
>>>>    int mul_required = 0;
>>>> -    double val, mul, integral, fraction;
>>>> +    bool use_strtod;
>>>> +    uint64_t valu;
>>>> +    double vald, mul, integral, fraction;
>>> 
>>> Note for later: @mul is double.
>>> 
>>>> +
>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>> +
>>>> +    /*
>>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>>> +     * which consumes more characters.
>>>> +     */
>>> 
>>> The comment is in a funny place.  I'd put it right before the
>>> qemu_strtod_finite() line.
>>> 
>>>> +    if (use_strtod) {
>>>> +        suffix = suffixd;
>>>> +        retval = retd;
>>>> +    } else {
>>>> +        suffix = suffixu;
>>>> +        retval = retu;
>>>> +    }
>>>> 
>>>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>    if (retval) {
>>>>        goto out;
>>>>    }
>>> 
>>> This is even more subtle than it looks.
>> 
>> But why it is even necessary?
>> 
>> The “contract” for the function used to be that it returned rounded values
>> beyond 2^53, which in itself is curious.
>> 
>> But now it’s a 6-dimensional matrix of hell with NaNs and barfnots, when the
>> name implies it’s simply doing a text to u64 conversion…
>> 
>> There is certainly a reason, but I’m really curious what it is :-)
> 
> It all goes back to commit 9f9b17a4f0 "Introduce strtosz() library
> function to convert a string to a byte count.".  To support "convenient"
> usage like "1.5G", it parses the number part with strtod().  This limits
> us to 53 bits of precision.  Larger sizes get rounded.
> 
> I guess the excuse for this was that when you're dealing with sizes that
> large (petabytes!), your least significant bits are zero anyway.
> 
> Regardless, the interface is *awful*.  We should've forced the author to
> spell it out in all its glory in a proper function contract.  That tends
> to cool the enthusiasm for "convenient" syntax amazingly fast.
> 
> The awful interface has been confusing people for close to a decade now.
> 
> What to do?

I see. Thanks for the rationale. I knew it had to make sense :-)

I’d probably avoid strtod even with the convenient syntax above.
Do you want 1.33e-6M to be allowed? Do we want to ever
accept or generate NaN or Inf values?

> 
> Tao Xu's patch tries to make the function do what its users expect,
> namely parse a bleepin' 64 bit integer, without breaking any of the
> "convenience" syntax.  Turns out that's amazingly subtle.  Are we making
> things less confusing or more?



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-17 14:12       ` Christophe de Dinechin
@ 2019-12-17 15:01         ` Markus Armbruster
  2019-12-18  2:29           ` Tao Xu
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-12-17 15:01 UTC (permalink / raw)
  To: Christophe de Dinechin; +Cc: Tao Xu, mdroth, ehabkost, qemu-devel

Christophe de Dinechin <dinechin@redhat.com> writes:

>> On 17 Dec 2019, at 15:08, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Christophe de Dinechin <dinechin@redhat.com> writes:
>> 
>>>> On 5 Dec 2019, at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>>>> 
>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>> 
>>>>> Parse input string both as a double and as a uint64_t, then use the
>>>>> method which consumes more characters. Update the related test cases.
>>>>> 
>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>> ---
>>>> [...]
>>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>>> index 77acadc70a..b08058c57c 100644
>>>>> --- a/util/cutils.c
>>>>> +++ b/util/cutils.c
>>>>> @@ -212,24 +212,43 @@ 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;
>>>>> +    int retval, retd, retu;
>>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>>    unsigned char c;
>>>>>    int mul_required = 0;
>>>>> -    double val, mul, integral, fraction;
>>>>> +    bool use_strtod;
>>>>> +    uint64_t valu;
>>>>> +    double vald, mul, integral, fraction;
>>>> 
>>>> Note for later: @mul is double.
>>>> 
>>>>> +
>>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>>> +
>>>>> +    /*
>>>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>>>> +     * which consumes more characters.
>>>>> +     */
>>>> 
>>>> The comment is in a funny place.  I'd put it right before the
>>>> qemu_strtod_finite() line.
>>>> 
>>>>> +    if (use_strtod) {
>>>>> +        suffix = suffixd;
>>>>> +        retval = retd;
>>>>> +    } else {
>>>>> +        suffix = suffixu;
>>>>> +        retval = retu;
>>>>> +    }
>>>>> 
>>>>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>>    if (retval) {
>>>>>        goto out;
>>>>>    }
>>>> 
>>>> This is even more subtle than it looks.
>>> 
>>> But why it is even necessary?
>>> 
>>> The “contract” for the function used to be that it returned rounded values
>>> beyond 2^53, which in itself is curious.
>>> 
>>> But now it’s a 6-dimensional matrix of hell with NaNs and barfnots, when the
>>> name implies it’s simply doing a text to u64 conversion…
>>> 
>>> There is certainly a reason, but I’m really curious what it is :-)
>> 
>> It all goes back to commit 9f9b17a4f0 "Introduce strtosz() library
>> function to convert a string to a byte count.".  To support "convenient"
>> usage like "1.5G", it parses the number part with strtod().  This limits
>> us to 53 bits of precision.  Larger sizes get rounded.
>> 
>> I guess the excuse for this was that when you're dealing with sizes that
>> large (petabytes!), your least significant bits are zero anyway.
>> 
>> Regardless, the interface is *awful*.  We should've forced the author to
>> spell it out in all its glory in a proper function contract.  That tends
>> to cool the enthusiasm for "convenient" syntax amazingly fast.
>> 
>> The awful interface has been confusing people for close to a decade now.
>> 
>> What to do?
>
> I see. Thanks for the rationale. I knew it had to make sense :-)

For a value of "sense"...

> I’d probably avoid strtod even with the convenient syntax above.
> Do you want 1.33e-6M to be allowed? Do we want to ever
> accept or generate NaN or Inf values?

NaN or Inf definitely not.  That's why we use qemu_strtod_finite()
before and after the patch.

No sane person should ever use 1.33e-6M.  Or even 1.1k (which yields
1126, rounded silently from machine number 1126.4000000000001, which
approximates the true value 1126.4).

Certain fractions are actually sane.  1.5k denotes a perfectly fine
integer, which the code manages not to screw up.  I'd recommend against
using fractions regardless.

What usage are we prepared to break?  What kind of confusion are we
willing to bear?  Those are the questions.

>> Tao Xu's patch tries to make the function do what its users expect,
>> namely parse a bleepin' 64 bit integer, without breaking any of the
>> "convenience" syntax.  Turns out that's amazingly subtle.  Are we making
>> things less confusing or more?



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-17 10:25     ` Markus Armbruster
@ 2019-12-18  1:33       ` Tao Xu
  2019-12-18  5:26         ` Tao Xu
  2019-12-18 21:49         ` Eric Blake
  0 siblings, 2 replies; 15+ messages in thread
From: Tao Xu @ 2019-12-18  1:33 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: mdroth, qemu-devel, ehabkost

On 12/17/2019 6:25 PM, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> On 12/5/19 11:29 PM, Markus Armbruster wrote:
>>> Tao Xu <tao3.xu@intel.com> writes:
>>>
>>>> Parse input string both as a double and as a uint64_t, then use the
>>>> method which consumes more characters. Update the related test cases.
>>>>
>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>> ---
>>> [...]
>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>> index 77acadc70a..b08058c57c 100644
>>>> --- a/util/cutils.c
>>>> +++ b/util/cutils.c
>>>> @@ -212,24 +212,43 @@ 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;
>>>> +    int retval, retd, retu;
>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>        unsigned char c;
>>>>        int mul_required = 0;
>>>> -    double val, mul, integral, fraction;
>>>> +    bool use_strtod;
>>>> +    uint64_t valu;
>>>> +    double vald, mul, integral, fraction;
>>>
>>> Note for later: @mul is double.
>>>
>>>> +
>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
> 
> Note for later: passing 0 to base accepts octal and hexadecimal
> integers.
> 
>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>> +
>>>> +    /*
>>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>>> +     * which consumes more characters.
>>>> +     */
>>>
>>> The comment is in a funny place.  I'd put it right before the
>>> qemu_strtod_finite() line.
>>>
>>>> +    if (use_strtod) {
>>>> +        suffix = suffixd;
>>>> +        retval = retd;
>>>> +    } else {
>>>> +        suffix = suffixu;
>>>> +        retval = retu;
>>>> +    }
>>>>    -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>        if (retval) {
>>>>            goto out;
>>>>        }
>>>
>>> This is even more subtle than it looks.
>>>
>>> A close reading of the function contracts leads to three cases for each
>>> conversion:
>>>
>>> * parse error (including infinity and NaN)
>>>
>>>     @retu / @retd is -EINVAL
>>>     @valu / @vald is uninitialized
>>>     @suffixu / @suffixd is @nptr
>>>
>>> * range error
>>>
>>>     @retu / @retd is -ERANGE
>>>     @valu / @vald is our best approximation of the conversion result
>>>     @suffixu / @suffixd points to the first character not consumed by the
>>>     conversion.
>>>
>>>     Sub-cases:
>>>
>>>     - uint64_t overflow
>>>
>>>       We know the conversion result exceeds UINT64_MAX.
>>>
>>>     - double overflow
>>>
>>>       we know the conversion result's magnitude exceeds the largest
>>>       representable finite double DBL_MAX.
>>>
>>>     - double underflow
>>>
>>>       we know the conversion result is close to zero (closer than DBL_MIN,
>>>       the smallest normalized positive double).
>>>
>>> * success
>>>
>>>     @retu / @retd is 0
>>>     @valu / @vald is the conversion result
>>>     @suffixu / @suffixd points to the first character not consumed by the
>>>     conversion.
>>>
>>> This leads to a matrix (parse error, uint64_t overflow, success) x
>>> (parse error, double overflow, double underflow, success).  We need to
>>> check the code does what we want for each element of this matrix, and
>>> document any behavior that's not perfectly obvious.
>>>
>>> (success, success): we pick uint64_t if qemu_strtou64() consumed more
>>> characters than qemu_strtod_finite(), else double.  "More" is important
>>> here; when they consume the same characters, we *need* to use the
>>> uint64_t result.  Example: for "18446744073709551615", we need to use
>>> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
>>> for "18446744073709551616.", we need to use the double.  Good.
> 
> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
> "0123.", we use 123.0, not 83.
> 
> Do we really want to accept octal and hexadecimal integers?
> 

Thank you for reminding me. Octal and hexadecimal may bring more 
confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and add 
test for input like "0123".

>>> (success, parse error) and (parse error, success): we pick the one that
>>> succeeds, because success consumes characters, and failure to parse does
>>> not.  Good.
>>>
>>> (parse error, parse error): neither consumes characters, so we pick
>>> uint64_t.  Good.
>>>
>>> (parse error, double overflow), (parse error, double underflow) and
>>> (uint64_t overflow, parse error): we pick the range error, because it
>>> consumes characters.  Good.
>>>
>>> These are the simple combinations.  The remainder are hairier: (success,
>>> double overflow), (success, double underflow), (uint64_t overflow,
>>> success).  I lack the time to analyze them today.  Must be done before
>>> we take this patch.  Any takers?
>>
>> (success, double overflow), (success, double underflow), pick double
>> overflow error, return -ERANGE. Because it consumes
>> characters. Example: for "1.79769e+309", qemu_strtou64 consumes "1",
>> and prases as uint64_t; but qemu_strtod_finite return -ERANGE and
>> consumes all characters. It is OK.
> 
> The only way to have double overflow when uint64_t succeeds is an
> exponent.  Double consumes the characters making up the exponent,
> uint64_t does not.  We use double.
> 
> The only way to have double underflow is with an exponent or a decimal
> point.  Double consumes their characters, uint64_t does not.  We use
> double.
> 
> Okay.
> 
>> (uint64_t overflow, success), consume the same characters, use the
>> uint64_t return -ERANGE. Note that even if qemu_strtod_finite can
>> parse these cases such as "18446744073709551617", but the result is
>> uint64_t so we also need to return -ERANGE. It is OK.
> 
> That's just one of two cases, I think.  The other one is when the
> overflowing integer is followed by an exponent or decimal point.  We use
> double then.  Converting the double to uint64_t overflows, except when a
> negative exponent brings the number into range.
> 
> Examples: "18446744073709551617" picks uint64_t overflow,
> "18446744073709551617.0" picks double success (but converting it to
> uint64_t below overflows), and "18446744073709551617e-10" picks double
> success (converted to 1844674407 below).
> 
> Okay.
> 
>> Thank you for your analysis and suggestion. I will add more test cases
>> to cover some of these analysis.
> 
> Good move.
> 
> 
Thank you for your further analysis.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-17 15:01         ` Markus Armbruster
@ 2019-12-18  2:29           ` Tao Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Tao Xu @ 2019-12-18  2:29 UTC (permalink / raw)
  To: Markus Armbruster, Christophe de Dinechin; +Cc: mdroth, ehabkost, qemu-devel

On 12/17/2019 11:01 PM, Markus Armbruster wrote:
> Christophe de Dinechin <dinechin@redhat.com> writes:
> 
>>> On 17 Dec 2019, at 15:08, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Christophe de Dinechin <dinechin@redhat.com> writes:
>>>
>>>>> On 5 Dec 2019, at 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>>>
>>>>>> Parse input string both as a double and as a uint64_t, then use the
>>>>>> method which consumes more characters. Update the related test cases.
>>>>>>
>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>>> ---
>>>>> [...]
>>>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>>>> index 77acadc70a..b08058c57c 100644
>>>>>> --- a/util/cutils.c
>>>>>> +++ b/util/cutils.c
>>>>>> @@ -212,24 +212,43 @@ 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;
>>>>>> +    int retval, retd, retu;
>>>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>>>     unsigned char c;
>>>>>>     int mul_required = 0;
>>>>>> -    double val, mul, integral, fraction;
>>>>>> +    bool use_strtod;
>>>>>> +    uint64_t valu;
>>>>>> +    double vald, mul, integral, fraction;
>>>>>
>>>>> Note for later: @mul is double.
>>>>>
>>>>>> +
>>>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Parse @nptr both as a double and as a uint64_t, then use the method
>>>>>> +     * which consumes more characters.
>>>>>> +     */
>>>>>
>>>>> The comment is in a funny place.  I'd put it right before the
>>>>> qemu_strtod_finite() line.
>>>>>
>>>>>> +    if (use_strtod) {
>>>>>> +        suffix = suffixd;
>>>>>> +        retval = retd;
>>>>>> +    } else {
>>>>>> +        suffix = suffixu;
>>>>>> +        retval = retu;
>>>>>> +    }
>>>>>>
>>>>>> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>>>     if (retval) {
>>>>>>         goto out;
>>>>>>     }
>>>>>
>>>>> This is even more subtle than it looks.
>>>>
>>>> But why it is even necessary?
>>>>
>>>> The “contract” for the function used to be that it returned rounded values
>>>> beyond 2^53, which in itself is curious.
>>>>
>>>> But now it’s a 6-dimensional matrix of hell with NaNs and barfnots, when the
>>>> name implies it’s simply doing a text to u64 conversion…
>>>>
>>>> There is certainly a reason, but I’m really curious what it is :-)
>>>
>>> It all goes back to commit 9f9b17a4f0 "Introduce strtosz() library
>>> function to convert a string to a byte count.".  To support "convenient"
>>> usage like "1.5G", it parses the number part with strtod().  This limits
>>> us to 53 bits of precision.  Larger sizes get rounded.
>>>
>>> I guess the excuse for this was that when you're dealing with sizes that
>>> large (petabytes!), your least significant bits are zero anyway.
>>>
>>> Regardless, the interface is *awful*.  We should've forced the author to
>>> spell it out in all its glory in a proper function contract.  That tends
>>> to cool the enthusiasm for "convenient" syntax amazingly fast.
>>>
>>> The awful interface has been confusing people for close to a decade now.
>>>
>>> What to do?
>>
>> I see. Thanks for the rationale. I knew it had to make sense :-)
> 
> For a value of "sense"...
> 
>> I’d probably avoid strtod even with the convenient syntax above.
>> Do you want 1.33e-6M to be allowed? Do we want to ever
>> accept or generate NaN or Inf values?
> 
> NaN or Inf definitely not.  That's why we use qemu_strtod_finite()
> before and after the patch.
> 
> No sane person should ever use 1.33e-6M.  Or even 1.1k (which yields
> 1126, rounded silently from machine number 1126.4000000000001, which
> approximates the true value 1126.4).
> 
> Certain fractions are actually sane.  1.5k denotes a perfectly fine
> integer, which the code manages not to screw up.  I'd recommend against
> using fractions regardless.
> 
> What usage are we prepared to break?  What kind of confusion are we
> willing to bear?  Those are the questions.
> 
>>> Tao Xu's patch tries to make the function do what its users expect,
>>> namely parse a bleepin' 64 bit integer, without breaking any of the
>>> "convenience" syntax.  Turns out that's amazingly subtle.  Are we making
>>> things less confusing or more?
> 
Thanks for your explanation. I think another reason is build-in 'size' 
is really commonly used. May be someone use '-m 1.5G' to boot QEMU or 
write it to a config file.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-18  1:33       ` Tao Xu
@ 2019-12-18  5:26         ` Tao Xu
  2019-12-18 18:26           ` Markus Armbruster
  2019-12-18 21:49         ` Eric Blake
  1 sibling, 1 reply; 15+ messages in thread
From: Tao Xu @ 2019-12-18  5:26 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: mdroth, qemu-devel, ehabkost

On 12/18/2019 9:33 AM, Tao Xu wrote:
> On 12/17/2019 6:25 PM, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> On 12/5/19 11:29 PM, Markus Armbruster wrote:
>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>>
>>>>> Parse input string both as a double and as a uint64_t, then use the
>>>>> method which consumes more characters. Update the related test cases.
>>>>>
>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>> ---
>>>> [...]
>>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>>> index 77acadc70a..b08058c57c 100644
>>>>> --- a/util/cutils.c
>>>>> +++ b/util/cutils.c
>>>>> @@ -212,24 +212,43 @@ 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;
>>>>> +    int retval, retd, retu;
>>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>>        unsigned char c;
>>>>>        int mul_required = 0;
>>>>> -    double val, mul, integral, fraction;
>>>>> +    bool use_strtod;
>>>>> +    uint64_t valu;
>>>>> +    double vald, mul, integral, fraction;
>>>>
>>>> Note for later: @mul is double.
>>>>
>>>>> +
>>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>
>> Note for later: passing 0 to base accepts octal and hexadecimal
>> integers.
>>
>>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>>> +
>>>>> +    /*
>>>>> +     * Parse @nptr both as a double and as a uint64_t, then use 
>>>>> the method
>>>>> +     * which consumes more characters.
>>>>> +     */
>>>>
>>>> The comment is in a funny place.  I'd put it right before the
>>>> qemu_strtod_finite() line.
>>>>
>>>>> +    if (use_strtod) {
>>>>> +        suffix = suffixd;
>>>>> +        retval = retd;
>>>>> +    } else {
>>>>> +        suffix = suffixu;
>>>>> +        retval = retu;
>>>>> +    }
>>>>>    -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>>        if (retval) {
>>>>>            goto out;
>>>>>        }
>>>>
>>>> This is even more subtle than it looks.
>>>>
>>>> A close reading of the function contracts leads to three cases for each
>>>> conversion:
>>>>
>>>> * parse error (including infinity and NaN)
>>>>
>>>>     @retu / @retd is -EINVAL
>>>>     @valu / @vald is uninitialized
>>>>     @suffixu / @suffixd is @nptr
>>>>
>>>> * range error
>>>>
>>>>     @retu / @retd is -ERANGE
>>>>     @valu / @vald is our best approximation of the conversion result
>>>>     @suffixu / @suffixd points to the first character not consumed 
>>>> by the
>>>>     conversion.
>>>>
>>>>     Sub-cases:
>>>>
>>>>     - uint64_t overflow
>>>>
>>>>       We know the conversion result exceeds UINT64_MAX.
>>>>
>>>>     - double overflow
>>>>
>>>>       we know the conversion result's magnitude exceeds the largest
>>>>       representable finite double DBL_MAX.
>>>>
>>>>     - double underflow
>>>>
>>>>       we know the conversion result is close to zero (closer than 
>>>> DBL_MIN,
>>>>       the smallest normalized positive double).
>>>>
>>>> * success
>>>>
>>>>     @retu / @retd is 0
>>>>     @valu / @vald is the conversion result
>>>>     @suffixu / @suffixd points to the first character not consumed 
>>>> by the
>>>>     conversion.
>>>>
>>>> This leads to a matrix (parse error, uint64_t overflow, success) x
>>>> (parse error, double overflow, double underflow, success).  We need to
>>>> check the code does what we want for each element of this matrix, and
>>>> document any behavior that's not perfectly obvious.
>>>>
>>>> (success, success): we pick uint64_t if qemu_strtou64() consumed more
>>>> characters than qemu_strtod_finite(), else double.  "More" is important
>>>> here; when they consume the same characters, we *need* to use the
>>>> uint64_t result.  Example: for "18446744073709551615", we need to use
>>>> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
>>>> for "18446744073709551616.", we need to use the double.  Good.
>>
>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>> "0123.", we use 123.0, not 83.
>>
>> Do we really want to accept octal and hexadecimal integers?
>>
> 
> Thank you for reminding me. Octal and hexadecimal may bring more 
> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and add 
> test for input like "0123".
> 

Hi Markus,

After I use qemu_strtou64(nptr, &suffixu, 10, &valu), it cause another 
question. Because qemu_strtod_finite support hexadecimal input, so in 
this situation, it will parsed as double. It will also let large 
hexadecimal integers be rounded. So there may be two solution:

1: use qemu_strtou64(nptr, &suffixu, 0, &valu) and parse octal as 
decimal. This will keep hexadecimal valid as now.

"0123" --> 123; "0x123" --> 291

2: use qemu_strtou64(nptr, &suffixu, 10, &valu) and reject octal and 
decimal.

"0123" --> Error; "0x123" --> Error



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-18  5:26         ` Tao Xu
@ 2019-12-18 18:26           ` Markus Armbruster
  2019-12-19  7:43             ` Tao Xu
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-12-18 18:26 UTC (permalink / raw)
  To: Tao Xu; +Cc: mdroth, ehabkost, qemu-devel

Tao Xu <tao3.xu@intel.com> writes:

> On 12/18/2019 9:33 AM, Tao Xu wrote:
>> On 12/17/2019 6:25 PM, Markus Armbruster wrote:
[...]
>>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>>> "0123.", we use 123.0, not 83.
>>>
>>> Do we really want to accept octal and hexadecimal integers?
>>>
>>
>> Thank you for reminding me. Octal and hexadecimal may bring more
>> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and
>> add test for input like "0123".
>>
>
> Hi Markus,
>
> After I use qemu_strtou64(nptr, &suffixu, 10, &valu), it cause another
> question. Because qemu_strtod_finite support hexadecimal input, so in
> this situation, it will parsed as double. It will also let large
> hexadecimal integers be rounded. So there may be two solution:
>
> 1: use qemu_strtou64(nptr, &suffixu, 0, &valu) and parse octal as
> decimal. This will keep hexadecimal valid as now.
>
> "0123" --> 123; "0x123" --> 291

How would you make qemu_strtou64() parse octal as decimal?

> 2: use qemu_strtou64(nptr, &suffixu, 10, &valu) and reject octal and
> decimal.
>
> "0123" --> Error; "0x123" --> Error

How would you reject the 0x prefix?



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-18  1:33       ` Tao Xu
  2019-12-18  5:26         ` Tao Xu
@ 2019-12-18 21:49         ` Eric Blake
  1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2019-12-18 21:49 UTC (permalink / raw)
  To: Tao Xu, Markus Armbruster; +Cc: mdroth, ehabkost, qemu-devel

On 12/17/19 7:33 PM, Tao Xu wrote:

>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>> "0123.", we use 123.0, not 83.
>>
>> Do we really want to accept octal and hexadecimal integers?
>>
> 
> Thank you for reminding me. Octal and hexadecimal may bring more 
> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and add 
> test for input like "0123".

Note that JSON does not permit octal numbers, but ALSO does not permit 
'0123' as a valid JSON number.  Of course, this parser is intended for 
human users rather than a JSON parser, so silently accepting it as 
decimal 123 is probably okay, but it is worth remembering that decisions 
are not trivial here.

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-18 18:26           ` Markus Armbruster
@ 2019-12-19  7:43             ` Tao Xu
  2019-12-19 10:15               ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Tao Xu @ 2019-12-19  7:43 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: mdroth, ehabkost, qemu-devel

On 12/19/2019 2:26 AM, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> On 12/18/2019 9:33 AM, Tao Xu wrote:
>>> On 12/17/2019 6:25 PM, Markus Armbruster wrote:
> [...]
>>>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>>>> "0123.", we use 123.0, not 83.
>>>>
>>>> Do we really want to accept octal and hexadecimal integers?
>>>>
>>>
>>> Thank you for reminding me. Octal and hexadecimal may bring more
>>> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and
>>> add test for input like "0123".
>>>
>>
>> Hi Markus,
>>
>> After I use qemu_strtou64(nptr, &suffixu, 10, &valu), it cause another
>> question. Because qemu_strtod_finite support hexadecimal input, so in
>> this situation, it will parsed as double. It will also let large
>> hexadecimal integers be rounded. So there may be two solution:
>>
>> 1: use qemu_strtou64(nptr, &suffixu, 0, &valu) and parse octal as
>> decimal. This will keep hexadecimal valid as now.
>>
>> "0123" --> 123; "0x123" --> 291
> 
> How would you make qemu_strtou64() parse octal as decimal?

How about this solution, set @base as variable, if we detect 
hexadecimal, we use 0, then can prase decimal as u64, else we use 10, 
then can prase octal as decimal, because 0 prefix will be ignored in 
qemu_strtou64(nptr, &suffixu, 10, &valu);

     const char *p = nptr;
     while (qemu_isspace(*p)) {
        p++;
     }
     if (*p == '0' && (qemu_toupper(*(p+1)) == 'X' ||) {
         base = 0;
     } else {
         base = 10;
     }

     retd = qemu_strtod_finite(nptr, &suffixd, &vald);
     retu = qemu_strtou64(nptr, &suffixu, base, &valu);
     use_strtod = strlen(suffixd) < strlen(suffixu);

     if (use_strtod) {
         endptr = suffixd;
         retval = retd;
     } else {
         endptr = suffixu;
         retval = retu;
     }
> 
>> 2: use qemu_strtou64(nptr, &suffixu, 10, &valu) and reject octal and
>> decimal.
>>
>> "0123" --> Error; "0x123" --> Error
> 
> How would you reject the 0x prefix?
> 
How about check the first&second character is '0' and 'x' and then 
return -EINVAL.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits
  2019-12-19  7:43             ` Tao Xu
@ 2019-12-19 10:15               ` Markus Armbruster
  0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2019-12-19 10:15 UTC (permalink / raw)
  To: Tao Xu; +Cc: qemu-devel, Christophe de Dinechin, mdroth, ehabkost

Tao Xu <tao3.xu@intel.com> writes:

> On 12/19/2019 2:26 AM, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> On 12/18/2019 9:33 AM, Tao Xu wrote:
>>>> On 12/17/2019 6:25 PM, Markus Armbruster wrote:
>> [...]
>>>>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>>>>> "0123.", we use 123.0, not 83.
>>>>>
>>>>> Do we really want to accept octal and hexadecimal integers?
>>>>>
>>>>
>>>> Thank you for reminding me. Octal and hexadecimal may bring more
>>>> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and
>>>> add test for input like "0123".
>>>>
>>>
>>> Hi Markus,
>>>
>>> After I use qemu_strtou64(nptr, &suffixu, 10, &valu), it cause another
>>> question. Because qemu_strtod_finite support hexadecimal input, so in
>>> this situation, it will parsed as double. It will also let large
>>> hexadecimal integers be rounded. So there may be two solution:
>>>
>>> 1: use qemu_strtou64(nptr, &suffixu, 0, &valu) and parse octal as
>>> decimal. This will keep hexadecimal valid as now.
>>>
>>> "0123" --> 123; "0x123" --> 291
>>
>> How would you make qemu_strtou64() parse octal as decimal?
>
> How about this solution, set @base as variable, if we detect
> hexadecimal, we use 0, then can prase decimal as u64, else we use 10,
> then can prase octal as decimal, because 0 prefix will be ignored in
> qemu_strtou64(nptr, &suffixu, 10, &valu);
>
>     const char *p = nptr;
>     while (qemu_isspace(*p)) {
>        p++;
>     }
>     if (*p == '0' && (qemu_toupper(*(p+1)) == 'X' ||) {
>         base = 0;
>     } else {
>         base = 10;
>     }
>
>     retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>     retu = qemu_strtou64(nptr, &suffixu, base, &valu);
>     use_strtod = strlen(suffixd) < strlen(suffixu);
>
>     if (use_strtod) {
>         endptr = suffixd;
>         retval = retd;
>     } else {
>         endptr = suffixu;
>         retval = retu;
>     }

You skip whitespace, but neglect to skip the sign.

Peeking into the input to predict what qemu_strtou64() will do feels
unadvisable.

We could try both base 10 and 16, and use whatever consumes more
characters, but that's even more complicated.

This function's contract is *terrible*.  We've tried to improve on it,
but so far only managed to make it more complex and differently
terrible.

Do we really, really, really need full precision?

If no, let's flee this swamp without looking back.

If yes, what's the *simplest* solution that provides it?

>>> 2: use qemu_strtou64(nptr, &suffixu, 10, &valu) and reject octal and
>>> decimal.
>>>
>>> "0123" --> Error; "0x123" --> Error
>>
>> How would you reject the 0x prefix?
>>
> How about check the first&second character is '0' and 'x' and then
> return -EINVAL.



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05  2:14 [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits Tao Xu
2019-12-05 15:29 ` Markus Armbruster
2019-12-09  5:38   ` Tao Xu
2019-12-17 10:25     ` Markus Armbruster
2019-12-18  1:33       ` Tao Xu
2019-12-18  5:26         ` Tao Xu
2019-12-18 18:26           ` Markus Armbruster
2019-12-19  7:43             ` Tao Xu
2019-12-19 10:15               ` Markus Armbruster
2019-12-18 21:49         ` Eric Blake
2019-12-17 12:04   ` Christophe de Dinechin
2019-12-17 14:08     ` Markus Armbruster
2019-12-17 14:12       ` Christophe de Dinechin
2019-12-17 15:01         ` Markus Armbruster
2019-12-18  2:29           ` Tao Xu

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git