Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Daniel P . Berrangé]

On Thu, Mar 11, 2021 at 02:07:02PM -0600, Eric Blake wrote:
> Not all floating point fractions are precise.  For example, the two
> nearest 32-bit IEEE float values for 0.345 are 0.344999998808 and
> 0.34500002861, with the lower one being closer.  When our scaling unit
> is 1000, that in turn can produce instances of double rounding (our
> first when truncating to get the floating point fraction compared to
> what the user typed, the second in converting the result of the
> multiplication back to an integer), resulting in a final result 1 byte
> smaller than the intuitive integer.
> 
> For the actual test failure encountered on gitlab cross-i386-user, we
> can clean things up by adding in DBL_EPSILON (with IEEE double, that
> is 2^-53) for all values on a scale smaller than Petabytes (that is
> 2^50), where our introduced error is not enough to add a full byte,
> but will be enough to cause the subsequent multiplication to overshoot
> rather than undershoot the nearest integer.  And ultimately, we've
> already documented that fractional values are for human convenience:
> if a user is worried about byte-level precision when specifying more
> than 50 bits of sizing, they should already be specifying things in
> bytes rather than fractions.
> 
> Fixes: cf923b783efd5 (utils: Improve qemu_strtosz() to have 64 bits of 
> precision)
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I'm not actually sure how to kick off a gitlab CI run of this to see if
> it fixes the failure originally reported at
> https://gitlab.com/qemu-project/qemu/-/jobs/1090685134
> Pointers welcome!

Create a gitlab.com account, and fork the QEMU repo.

Then simply push your branch to your QEMU fork. Pipeline will run
automagically and be visible in

https://gitlab.com/<your user name>/qemu/-/pipelines

> An alternative patch might be writing (uint64_t)(fraction * mul + 0.5)
> (that is, introduce the fudge factor after the multiplication instead
> of before).  Preferences?

No pref from me if both achieve the same end result.

>  util/cutils.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index d89a40a8c325..c124d8165f57 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/host-utils.h"
>  #include <math.h>
> +#include <float.h>
> 
>  #include "qemu-common.h"
>  #include "qemu/sockets.h"
> @@ -329,6 +330,15 @@ static int do_strtosz(const char *nptr, const char **end,
>                          "is deprecated: %s", nptr);
>          }
>          endptr++;
> +        /*
> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
> +         * all scales less than P (2^50), so that things like
> +         * 12.345M with unit 1000 produce 12345000 instead of
> +         * 12344999.
> +         */
> +        if (mul > 1e49) {
> +            fraction += DBL_EPSILON;
> +        }

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


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