Re: [PATCH RESEND v2] util/cutils: Expand do_strtosz parsing precision to 64 bits

[Tao Xu]

On 12/17/2019 7:44 PM, Christophe de Dinechin wrote:
> > On 9 Dec 2019, at 09:30, Tao Xu <address@hidden> wrote:
> >
> > 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 <address@hidden>
> > ---
> >
> > Changes in v2:
> >     - Resend to use double small than DBL_MIN
> >     - Add more test case for double overflow and underflow.
> >     - Set mul as int64_t (Markus)
> >     - Restore endptr (Markus)
> > ---
> > tests/test-cutils.c    | 37 +++++++----------------
> > tests/test-keyval.c    | 47 +++++------------------------
> > tests/test-qemu-opts.c | 39 +++++-------------------
> > util/cutils.c          | 67 +++++++++++++++++++++++++++++++-----------
> > 4 files changed, 75 insertions(+), 115 deletions(-)
[...]
> > +    /*
> > +     * Parse @nptr both as a double and as a uint64_t, then use the method
> > +     * which consumes more characters.
> > +     */
>
> Why do ever need to parse as double if you have uint64?

Because we want to keep do_strtosz Compatible with double input (such as 
1.5k).
> > +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
> > +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
> > +    use_strtod = strlen(suffixd) < strlen(suffixu);
>
> You could simply compare suffixd and suffixu:
>
> use_strtod = suffixd > suffixu;

Thank you for your suggestion.
> > +
> > +    if (use_strtod) {
> > +        endptr = suffixd;
> > +        retval = retd;
> > +    } else {
> > +        endptr = 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;
> >      mul = suffix_mul(c, unit);
> > @@ -238,17 +258,30 @@ 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 / mul < valu) {
> > +            retval = -ERANGE;
> > +            goto out;
> > +        }
> > +        *result = valu * mul;
> >      }
> > -    *result = val * mul;
> >      retval = 0;
> >
> > out:
> > --
> > 2.20.1