| Message ID | 20210317072216.16316-2-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/15] utils: Use fixed-point arithmetic in qemu_strtosz | expand |
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); > }
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); >> }
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.
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).
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)?
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: