diff mbox series

[v17,02/14] util/cutils: Use qemu_strtold_finite to parse size

Message ID 20191122074826.1373-3-tao3.xu@intel.com
State New
Headers show
Series Build ACPI Heterogeneous Memory Attribute Table (HMAT) | expand

Commit Message

Tao Xu Nov. 22, 2019, 7:48 a.m. UTC 
Support full 64bit precision, modify related test cases.

Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v17.
---
 tests/test-cutils.c    | 41 +++++-------------------------------
 tests/test-keyval.c    | 47 +++++++-----------------------------------
 tests/test-qemu-opts.c | 39 +++++++----------------------------
 util/cutils.c          | 13 +++++-------
 4 files changed, 24 insertions(+), 116 deletions(-)

Comments

Markus Armbruster Nov. 25, 2019, 6:56 a.m. UTC | #1 
Tao Xu <tao3.xu@intel.com> writes:

> Support full 64bit precision, modify related test cases.

That's not true in general: long double need not be any wider than
double.

It might be true on the host machines we support, but I don't know.  If
we decide to rely on it, we better make the build fail when the host
machine doesn't meet our expectations, preferably in configure.

>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
[...]
> diff --git a/util/cutils.c b/util/cutils.c
> index 5db3b2add5..d94a468954 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -216,13 +216,13 @@ static int do_strtosz(const char *nptr, const char **end,
>      const char *endptr;
>      unsigned char c;
>      int mul_required = 0;
> -    double val, mul, integral, fraction;
> +    long double val, mul, integral, fraction;
>  
> -    retval = qemu_strtod_finite(nptr, &endptr, &val);
> +    retval = qemu_strtold_finite(nptr, &endptr, &val);
>      if (retval) {
>          goto out;
>      }
> -    fraction = modf(val, &integral);
> +    fraction = modfl(val, &integral);
>      if (fraction != 0) {
>          mul_required = 1;
>      }
> @@ -238,11 +238,8 @@ static int do_strtosz(const char *nptr, const char **end,
>          retval = -EINVAL;
>          goto out;
>      }
> -    /*
> -     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
> -     * through double (53 bits of precision).
> -     */
> -    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
> +    /* Values > UINT64_MAX overflow uint64_t */
> +    if ((val * mul > UINT64_MAX) || val < 0) {
>          retval = -ERANGE;
>          goto out;
>      }

Not portable.  If it was, we'd have made this changd long ago :)
Tao Xu Nov. 26, 2019, 8:31 a.m. UTC | #2 
On 11/25/2019 2:56 PM, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> Support full 64bit precision, modify related test cases.
> 
> That's not true in general: long double need not be any wider than
> double.
> 
> It might be true on the host machines we support, but I don't know.  If
> we decide to rely on it, we better make the build fail when the host
> machine doesn't meet our expectations, preferably in configure.
> 
[...]
>> -    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
>> +    /* Values > UINT64_MAX overflow uint64_t */
>> +    if ((val * mul > UINT64_MAX) || val < 0) {
>>           retval = -ERANGE;
>>           goto out;
>>       }
> 
> Not portable.  If it was, we'd have made this changd long ago :)
> 

OK. So the suitable solution is what you suggested in v14?

"A possible alternative is to parse the numeric part both as a double 
and as a 64 bit unsigned integer, then use whatever consumes more 
characters.  This enables providing full 64 bits unless you actually use
a fraction."

I will try this way.
Markus Armbruster Nov. 26, 2019, 9:33 a.m. UTC | #3 
Tao Xu <tao3.xu@intel.com> writes:

> On 11/25/2019 2:56 PM, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> Support full 64bit precision, modify related test cases.
>>
>> That's not true in general: long double need not be any wider than
>> double.
>>
>> It might be true on the host machines we support, but I don't know.  If
>> we decide to rely on it, we better make the build fail when the host
>> machine doesn't meet our expectations, preferably in configure.
>>
> [...]
>>> -    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
>>> +    /* Values > UINT64_MAX overflow uint64_t */
>>> +    if ((val * mul > UINT64_MAX) || val < 0) {
>>>           retval = -ERANGE;
>>>           goto out;
>>>       }
>>
>> Not portable.  If it was, we'd have made this changd long ago :)
>>
>
> OK. So the suitable solution is what you suggested in v14?
>
> "A possible alternative is to parse the numeric part both as a double
> and as a 64 bit unsigned integer, then use whatever consumes more
> characters.  This enables providing full 64 bits unless you actually
> use
> a fraction."

Yes, that bypasses the portability issue.

> I will try this way.

Go ahead.
diff mbox series

Patch

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 1aa8351520..465514b85f 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1970,40 +1970,19 @@  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() */
+    /* Note: precision is 64 bits (UINT64_MAX) */
 
-    str = "9007199254740991"; /* 2^53-1 */
+    str = "18446744073709551614"; /* UINT64_MAX - 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);
-
-    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(endptr == str + 16);
-
-    str = "18446744073709549568"; /* 0xfffffffffffff800 (53 msbs set) */
-    err = qemu_strtosz(str, &endptr, &res);
-    g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, 0xfffffffffffff800);
+    g_assert_cmpint(res, ==, 0xfffffffffffffffe);
     g_assert(endptr == str + 20);
 
-    str = "18446744073709550591"; /* 0xfffffffffffffbff */
+    str = "18446744073709551615"; /* 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 +2124,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..1236bf6b7d 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 5db3b2add5..d94a468954 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -216,13 +216,13 @@  static int do_strtosz(const char *nptr, const char **end,
     const char *endptr;
     unsigned char c;
     int mul_required = 0;
-    double val, mul, integral, fraction;
+    long double val, mul, integral, fraction;
 
-    retval = qemu_strtod_finite(nptr, &endptr, &val);
+    retval = qemu_strtold_finite(nptr, &endptr, &val);
     if (retval) {
         goto out;
     }
-    fraction = modf(val, &integral);
+    fraction = modfl(val, &integral);
     if (fraction != 0) {
         mul_required = 1;
     }
@@ -238,11 +238,8 @@  static int do_strtosz(const char *nptr, const char **end,
         retval = -EINVAL;
         goto out;
     }
-    /*
-     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
-     * through double (53 bits of precision).
-     */
-    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
+    /* Values > UINT64_MAX overflow uint64_t */
+    if ((val * mul > UINT64_MAX) || val < 0) {
         retval = -ERANGE;
         goto out;
     }