Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[David Hildenbrand <david@redhat.com>]

On 15.11.18 17:22, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>>> Let's provide a wrapper for strtod().
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   include/qemu/cutils.h |  2 ++
>>>   util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 40 insertions(+)
>>>
>>
>>
>>> +
>>> +/**
>>> + * Convert string @nptr to a finite double.
>>> + *
>>> + * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
>>> + * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.
>>
>> s/rejcted/rejected/
> 
> Also, just overflow.  Floating-point underflow is when a computation's
> mathematical result is too close to zero to be represented without
> extraordinary rounding error.

Indeed, as the "man strod" states
"... would cause overflow, plus or minus HUGE_VAL (HUGE_VALF, HUGE_VALL)
is returned (according to the sign of the value)"

> 
> Skip this paragraph unless you're ready to nerd out.  IEEE 754 section
> 7.5 defines underflow to happen
> 
>     [...] either
> 
>     a) after rounding — when a non-zero result computed as though the
>     exponent range were unbounded would lie strictly between ±b^emin,
>     or
> 
>     b) before rounding — when a non-zero result computed as though both
>     the exponent range and the precision were unbounded would lie
>     strictly between ±b^emin.
> 
> where b^emin is the smallest normal number.  
> 
> The "Works like qemu_strtoul()" is a bit lazy.  I guess it works like
> qemu_strtoul() in the sense that it adds to strtod() what qemu_strtoul()
> adds to strtoul().  I consciously didn't take a similar shortcut in
> commit 4295f879bec: I documented both qemu_strtol() and qemu_strtoul()
> in longhand, and used "Works like" shorthand only where that's actually
> the case: qemu_strtoll() works like qemu_strtol(), and qemu_strtoull()
> works like qemu_strtoul().  I'd prefer longhand for qemu_strtod().  It
> costs us a few lines, but it results in a clearer contract.

/**
 * Convert string @nptr to a double.
  *
 * This is a wrapper around strtod() that is harder to misuse.
 * Semantics of @nptr and @endptr match strtod() with differences
 * noted below.
 *
 * @nptr may be null, and no conversion is performed then.
 *
 * If no conversion is performed, store @nptr in *@endptr and return
 * -EINVAL.
 *
 * If @endptr is null, and the string isn't fully converted, return
 * -EINVAL.  This is the case when the pointer that would be stored in
 * a non-null @endptr points to a character other than '\0'.
 *
 * If the conversion overflows @result, store +/-HUGE_VAL, depending on
 * the sign, in @result and return -ERANGE.
 *
 * Else store the converted value in @result, and return zero.
 */

> 
>>> + */
>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>> +{
>>> +    int ret = qemu_strtod(nptr, endptr, result);
>>
>> On overflow, result is set to HUGE_VAL (aka "inf") with ret set to
>> -ERANGE.  (The C standard uses HUGE_VAL rather than directly requiring
>> infinity on overflow, in order to cater to museum platforms where the
>> largest representable double is still finite; but no one develops qemu
>> on a non-IEEE machine these days so we know that HUGE_VAL == INF).
> 
> Aside: museum clauses like this one make the standard much harder to
> read than necessary.  I wish they'll purge them from C2X.
> 
>>> +
>>> +    if (!ret && !isfinite(*result)) {
>>> +        return -EINVAL;
>>> +    }
> 
> qemu_strtol() & friends leave *result alone when they return -EINVAL.
> This one doesn't.  Unlikely to hurt anyone, but I'd prefer to keep them
> consistent.

Will use a temporary and also properly set endptr in case we return
-EINVAL; And add a fully-blown description as noted above :)

> 
>> This check means that overflow ("1e9999") fails with -ERANGE, while
>> actual infinity ("inf") fails with -EINVAL, letting the user
>> distinguish between the two.  Still, I wonder if assigning a
>> non-finite value into result on -ERANGE is the wisest course of
>> action.  We'll just have to see in the next patches that use this.
> 
> I guess it's about as "wise" as qemu_strtol() storing LONG_MAX on
> integer overflow.
> 
> I'm fine with the semantics David picked, as long as they're spelled out
> in the function contract.

I think for now we're fine treating explicit "infinity" user input as
-EINVAL. We could return something like "HUGE_VAL - 1" along with
-ERANGE, but I guess for now this is overkill. Most callers will bail
out on -ERANGE either way. And if not, they have to make sure they can
deal with HUGE_VAL.

> 
>> With the typo fix,
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

-- 

Thanks,

David / dhildenb