diff mbox series

[PULL,v2,01/15] utils: Use fixed-point arithmetic in qemu_strtosz

Message ID 20210317072216.16316-2-alex.bennee@linaro.org
State Accepted
Commit 6567ba0c60d6e1366f7ac6e77665730e657e8eca
Headers show
Series misc fixes (strtoz, plugins, guest-loader) | expand

Commit Message

Alex Bennée March 17, 2021, 7:22 a.m. UTC
From: Richard Henderson <richard.henderson@linaro.org>


Once we've parsed the fractional value, extract it into an integral
64-bit fraction.  Perform the scaling with integer arithmetic, and
simplify the overflow detection.

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

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Message-Id: <20210315185117.1986240-2-richard.henderson@linaro.org>

-- 
2.20.1

Comments

Philippe Mathieu-Daudé March 17, 2021, 11:53 a.m. UTC | #1
Hi Alex,

On 3/17/21 8:22 AM, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>

> 

> Once we've parsed the fractional value, extract it into an integral

> 64-bit fraction.  Perform the scaling with integer arithmetic, and

> simplify the overflow detection.

> 

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

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Message-Id: <20210315185117.1986240-2-richard.henderson@linaro.org>


Something is odd with your tooling, the '---' separator is missing.

The covers' tag is OK although.

> 

> diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c

> index bad3a60993..e025b54c05 100644

> --- a/tests/unit/test-cutils.c

> +++ b/tests/unit/test-cutils.c

> @@ -2128,7 +2128,7 @@ static void test_qemu_strtosz_float(void)

>      str = "12.345M";

>      err = qemu_strtosz(str, &endptr, &res);

>      g_assert_cmpint(err, ==, 0);

> -    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));

> +    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB + 0.5));

>      g_assert(endptr == str + 7);

>  }
Alex Bennée March 17, 2021, 12:13 p.m. UTC | #2
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Alex,

>

> On 3/17/21 8:22 AM, Alex Bennée wrote:

>> From: Richard Henderson <richard.henderson@linaro.org>

>> 

>> Once we've parsed the fractional value, extract it into an integral

>> 64-bit fraction.  Perform the scaling with integer arithmetic, and

>> simplify the overflow detection.

>> 

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

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> Message-Id: <20210315185117.1986240-2-richard.henderson@linaro.org>

>

> Something is odd with your tooling, the '---' separator is missing.


Surely that's only when you have bellow the line comments? b4 strips
then when applying series.

>

> The covers' tag is OK although.

>

>> 

>> diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c

>> index bad3a60993..e025b54c05 100644

>> --- a/tests/unit/test-cutils.c

>> +++ b/tests/unit/test-cutils.c

>> @@ -2128,7 +2128,7 @@ static void test_qemu_strtosz_float(void)

>>      str = "12.345M";

>>      err = qemu_strtosz(str, &endptr, &res);

>>      g_assert_cmpint(err, ==, 0);

>> -    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));

>> +    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB + 0.5));

>>      g_assert(endptr == str + 7);

>>  }



-- 
Alex Bennée
Philippe Mathieu-Daudé March 17, 2021, 1:16 p.m. UTC | #3
On 3/17/21 1:13 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> 

>> Hi Alex,

>>

>> On 3/17/21 8:22 AM, Alex Bennée wrote:

>>> From: Richard Henderson <richard.henderson@linaro.org>

>>>

>>> Once we've parsed the fractional value, extract it into an integral

>>> 64-bit fraction.  Perform the scaling with integer arithmetic, and

>>> simplify the overflow detection.

>>>

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

>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>>> Message-Id: <20210315185117.1986240-2-richard.henderson@linaro.org>

>>

>> Something is odd with your tooling, the '---' separator is missing.

> 

> Surely that's only when you have bellow the line comments? b4 strips

> then when applying series.


Yes, the problem is your series doesn't apply on top of 7625a1ed013
("utils: Use fixed-point arithmetic in qemu_strtosz")

$ git am v2_20210317_alex_bennee_misc_fixes_strtoz_plugins_guest_loader.mbx
Applying: utils: Use fixed-point arithmetic in qemu_strtosz
error: patch failed: tests/unit/test-cutils.c:2128
error: tests/unit/test-cutils.c: patch does not apply
error: patch failed: util/cutils.c:275
error: util/cutils.c: patch does not apply
Patch failed at 0001 utils: Use fixed-point arithmetic in qemu_strtosz
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

But skipping this patch, the rest can be applied properly by git-am.
Eric Blake March 17, 2021, 1:52 p.m. UTC | #4
On 3/17/21 8:16 AM, Philippe Mathieu-Daudé wrote:

> Yes, the problem is your series doesn't apply on top of 7625a1ed013

> ("utils: Use fixed-point arithmetic in qemu_strtosz")

> 

> $ git am v2_20210317_alex_bennee_misc_fixes_strtoz_plugins_guest_loader.mbx

> Applying: utils: Use fixed-point arithmetic in qemu_strtosz

> error: patch failed: tests/unit/test-cutils.c:2128

> error: tests/unit/test-cutils.c: patch does not apply

> error: patch failed: util/cutils.c:275

> error: util/cutils.c: patch does not apply

> Patch failed at 0001 utils: Use fixed-point arithmetic in qemu_strtosz

> hint: Use 'git am --show-current-patch=diff' to see the failed patch

> When you have resolved this problem, run "git am --continue".

> If you prefer to skip this patch, run "git am --skip" instead.

> To restore the original branch and stop patching, run "git am --abort".

> 

> But skipping this patch, the rest can be applied properly by git-am.


Merging the same patch twice through two independent pull requests is
not a problem with git.  Applying the patches of a pull request is
different than applying a merge request directly (where you failed
trying to reapply a patch that is already present).  There's no need to
respin this pull request just to drop patch 1, but if there is another
reason to respin, rebasing the series will automatically drop patch 1
because it is already upstream through rth's pull request (as you noted,
commit 7625a1ed).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Alex Bennée March 17, 2021, 2:31 p.m. UTC | #5
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 3/17/21 1:13 PM, Alex Bennée wrote:

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

>> 

>>> Hi Alex,

>>>

>>> On 3/17/21 8:22 AM, Alex Bennée wrote:

>>>> From: Richard Henderson <richard.henderson@linaro.org>

>>>>

>>>> Once we've parsed the fractional value, extract it into an integral

>>>> 64-bit fraction.  Perform the scaling with integer arithmetic, and

>>>> simplify the overflow detection.

>>>>

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

>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>>>> Message-Id: <20210315185117.1986240-2-richard.henderson@linaro.org>

>>>

>>> Something is odd with your tooling, the '---' separator is missing.

>> 

>> Surely that's only when you have bellow the line comments? b4 strips

>> then when applying series.

>

> Yes, the problem is your series doesn't apply on top of 7625a1ed013

> ("utils: Use fixed-point arithmetic in qemu_strtosz")

>

> $ git am v2_20210317_alex_bennee_misc_fixes_strtoz_plugins_guest_loader.mbx

> Applying: utils: Use fixed-point arithmetic in qemu_strtosz

> error: patch failed: tests/unit/test-cutils.c:2128

> error: tests/unit/test-cutils.c: patch does not apply

> error: patch failed: util/cutils.c:275

> error: util/cutils.c: patch does not apply

> Patch failed at 0001 utils: Use fixed-point arithmetic in qemu_strtosz

> hint: Use 'git am --show-current-patch=diff' to see the failed patch

> When you have resolved this problem, run "git am --continue".

> If you prefer to skip this patch, run "git am --skip" instead.

> To restore the original branch and stop patching, run "git am --abort".

>

> But skipping this patch, the rest can be applied properly by git-am.


I can imagine git am might get confused, out of interest what about git
merge (as this is a PR and previously git is pretty smart about this)?

-- 
Alex Bennée
diff mbox series

Patch

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index bad3a60993..e025b54c05 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -2128,7 +2128,7 @@  static void test_qemu_strtosz_float(void)
     str = "12.345M";
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, 0);
-    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));
+    g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB + 0.5));
     g_assert(endptr == str + 7);
 }
 
diff --git a/util/cutils.c b/util/cutils.c
index d89a40a8c3..c442882b88 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -275,10 +275,9 @@  static int do_strtosz(const char *nptr, const char **end,
     int retval;
     const char *endptr, *f;
     unsigned char c;
-    bool mul_required = false, hex = false;
-    uint64_t val;
+    bool hex = false;
+    uint64_t val, valf = 0;
     int64_t mul;
-    double fraction = 0.0;
 
     /* Parse integral portion as decimal. */
     retval = qemu_strtou64(nptr, &endptr, 10, &val);
@@ -308,17 +307,19 @@  static int do_strtosz(const char *nptr, const char **end,
          * without fractional digits.  If we see an exponent, treat
          * the entire input as invalid instead.
          */
+        double fraction;
+
         f = endptr;
         retval = qemu_strtod_finite(f, &endptr, &fraction);
         if (retval) {
-            fraction = 0.0;
             endptr++;
         } else if (memchr(f, 'e', endptr - f) || memchr(f, 'E', endptr - f)) {
             endptr = nptr;
             retval = -EINVAL;
             goto out;
-        } else if (fraction != 0) {
-            mul_required = true;
+        } else {
+            /* Extract into a 64-bit fixed-point fraction. */
+            valf = (uint64_t)(fraction * 0x1p64);
         }
     }
     c = *endptr;
@@ -333,16 +334,35 @@  static int do_strtosz(const char *nptr, const char **end,
         mul = suffix_mul(default_suffix, unit);
         assert(mul > 0);
     }
-    if (mul == 1 && mul_required) {
-        endptr = nptr;
-        retval = -EINVAL;
-        goto out;
-    }
-    if (val > (UINT64_MAX - ((uint64_t) (fraction * mul))) / mul) {
-        retval = -ERANGE;
-        goto out;
+    if (mul == 1) {
+        /* When a fraction is present, a scale is required. */
+        if (valf != 0) {
+            endptr = nptr;
+            retval = -EINVAL;
+            goto out;
+        }
+    } else {
+        uint64_t valh, tmp;
+
+        /* Compute exact result: 64.64 x 64.0 -> 128.64 fixed point */
+        mulu64(&val, &valh, val, mul);
+        mulu64(&valf, &tmp, valf, mul);
+        val += tmp;
+        valh += val < tmp;
+
+        /* Round 0.5 upward. */
+        tmp = valf >> 63;
+        val += tmp;
+        valh += val < tmp;
+
+        /* Report overflow. */
+        if (valh != 0) {
+            retval = -ERANGE;
+            goto out;
+        }
     }
-    *result = val * mul + (uint64_t) (fraction * mul);
+
+    *result = val;
     retval = 0;
 
 out: