Let's provide a wrapper for strtod().

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/cutils.h |  2 ++
 util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 7071bfe2d4..756b41c193 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -146,6 +146,8 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
                   int64_t *result);
 int qemu_strtou64(const char *nptr, const char **endptr, int base,
                   uint64_t *result);
+int qemu_strtod(const char *nptr, const char **endptr, double *result);
+int qemu_strtod_finite(const char *nptr, const char **endptr, double *result);
 
 int parse_uint(const char *s, unsigned long long *value, char **endptr,
                int base);
diff --git a/util/cutils.c b/util/cutils.c
index 698bd315bd..c965dbfcad 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -544,6 +544,71 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
     return check_strtox_error(nptr, ep, endptr, errno);
 }
 
+/**
+ * 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, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return -ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return -ERANGE.
+ *
+ * Else store the converted value in @result, and return zero.
+ */
+int qemu_strtod(const char *nptr, const char **endptr, double *result)
+{
+    char *ep;
+
+    if (!nptr) {
+        if (endptr) {
+            *endptr = nptr;
+        }
+        return -EINVAL;
+    }
+
+    errno = 0;
+    *result = strtod(nptr, &ep);
+    return check_strtox_error(nptr, ep, endptr, errno);
+}
+
+/**
+ * Convert string @nptr to a finite double.
+ *
+ * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
+ * with -EINVAL and no conversion is performed.
+ */
+int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
+{
+    double tmp;
+    int ret;
+
+    ret = qemu_strtod(nptr, endptr, &tmp);
+    if (ret) {
+        return ret;
+    } else if (!isfinite(tmp)) {
+        if (endptr) {
+            *endptr = nptr;
+        }
+        return -EINVAL;
+    }
+
+    *result = tmp;
+    return ret;
+}
+
 /**
  * Searches for the first occurrence of 'c' in 's', and returns a pointer
  * to the trailing null byte if none was found.
-- 
2.17.2

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> Let's provide a wrapper for strtod().
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

This changed enough from v1 that I would have dropped R-b to ensure that 
reviewers notice the differences.

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/qemu/cutils.h |  2 ++
>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+)
> 

> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
> + * on the sign, and return -ERANGE.
> + *
> + * If the conversion underflows, store ±0.0 in @result, depending on the
> + * sign, and return -ERANGE.

The use of UTF-8 ± in one place but not both is odd.  I think we're at 
the point where UTF-8 comments are acceptable these days, rather than 
trying to keep our codebase ASCII-clean, so I don't care which way you 
resolve the inconsistency.

> +/**
> + * Convert string @nptr to a finite double.
> + *
> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
> + * with -EINVAL and no conversion is performed.
> + */
> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
> +{
> +    double tmp;
> +    int ret;
> +
> +    ret = qemu_strtod(nptr, endptr, &tmp);
> +    if (ret) {
> +        return ret;

So, if we overflow, we are returning -ERANGE but with nothing stored 
into *result.  This is different from qemu_strtod(), where a return of 
-ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).  That 
seems awkward.

> +    } else if (!isfinite(tmp)) {
> +        if (endptr) {
> +            *endptr = nptr;
> +        }
> +        return -EINVAL;

Rewinding back to the start of "inf" is interesting, but matches your 
documentation.

> +    }
> +
> +    *result = tmp;
> +    return ret;
> +}
> +

I think you still need to fix -ERANGE handling before I can give R-b.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> Let's provide a wrapper for strtod().
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> This changed enough from v1 that I would have dropped R-b to ensure
> that reviewers notice the differences.
>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  2 ++
>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 67 insertions(+)
>>
>
>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>> + * on the sign, and return -ERANGE.
>> + *
>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>> + * sign, and return -ERANGE.
>
> The use of UTF-8 ± in one place but not both is odd.  I think we're at
> the point where UTF-8 comments are acceptable these days, rather than
> trying to keep our codebase ASCII-clean, so I don't care which way you
> resolve the inconsistency.

217 out of 6455 git-controlled files contain non-ASCII characters.  53
of them are binary, and don't count.  In most text files, it's for
spelling names of authors properly in comments.  Ample precedence for
UTF-8 in comments, I'd say.

That said, I second Eric's call for consistency, with the slightest of
preferrences for plain ASCII.

I spotted UTF-8 in two error messages, which might still be unadvisable:

hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",

>> +/**
>> + * Convert string @nptr to a finite double.
>> + *
>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>> + * with -EINVAL and no conversion is performed.
>> + */
>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>> +{
>> +    double tmp;
>> +    int ret;
>> +
>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>> +    if (ret) {
>> +        return ret;
>
> So, if we overflow, we are returning -ERANGE but with nothing stored
> into *result.  This is different from qemu_strtod(), where a return of
> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
> That seems awkward.

Violates the contract's "like qemu_strtod()".

>> +    } else if (!isfinite(tmp)) {
>> +        if (endptr) {
>> +            *endptr = nptr;
>> +        }
>> +        return -EINVAL;
>
> Rewinding back to the start of "inf" is interesting, but matches your
> documentation.

Yes.  I like it.

>> +    }
>> +
>> +    *result = tmp;
>> +    return ret;
>> +}
>> +
>
> I think you still need to fix -ERANGE handling before I can give R-b.

On 20.11.18 21:07, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> Let's provide a wrapper for strtod().
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> This changed enough from v1 that I would have dropped R-b to ensure
>> that reviewers notice the differences.

Indeed, dropping it now ;)

>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   include/qemu/cutils.h |  2 ++
>>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 67 insertions(+)
>>>
>>
>>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>>> + * on the sign, and return -ERANGE.
>>> + *
>>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>>> + * sign, and return -ERANGE.
>>
>> The use of UTF-8 ± in one place but not both is odd.  I think we're at
>> the point where UTF-8 comments are acceptable these days, rather than
>> trying to keep our codebase ASCII-clean, so I don't care which way you
>> resolve the inconsistency.
> 
> 217 out of 6455 git-controlled files contain non-ASCII characters.  53
> of them are binary, and don't count.  In most text files, it's for
> spelling names of authors properly in comments.  Ample precedence for
> UTF-8 in comments, I'd say.
> 
> That said, I second Eric's call for consistency, with the slightest of
> preferrences for plain ASCII.

I'll just go with +/-. Thanks.

> 
> I spotted UTF-8 in two error messages, which might still be unadvisable:
> 
> hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
> hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
> 
>>> +/**
>>> + * Convert string @nptr to a finite double.
>>> + *
>>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>>> + * with -EINVAL and no conversion is performed.
>>> + */
>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>> +{
>>> +    double tmp;
>>> +    int ret;
>>> +
>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>> +    if (ret) {
>>> +        return ret;
>>
>> So, if we overflow, we are returning -ERANGE but with nothing stored
>> into *result.  This is different from qemu_strtod(), where a return of
>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>> That seems awkward.
> 
> Violates the contract's "like qemu_strtod()".

Right, I missed that. What about something like this:

int qemu_strtod_finite(const char *nptr, const char **endptr, double
*result)
{
    double tmp;
    int ret;

    ret = qemu_strtod(nptr, endptr, &tmp);
    if (!ret && !isfinite(tmp)) {
        if (endptr) {
            *endptr = nptr;
        }
        ret = -EINVAL;
    }

    if (ret != -EINVAL) {
        *result = tmp;
    }
    return ret;
}



-- 

Thanks,

David / dhildenb

On 11/21/18 4:35 AM, David Hildenbrand wrote:

>>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>>> +{
>>>> +    double tmp;
>>>> +    int ret;
>>>> +
>>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>>> +    if (ret) {
>>>> +        return ret;
>>>
>>> So, if we overflow, we are returning -ERANGE but with nothing stored
>>> into *result.  This is different from qemu_strtod(), where a return of
>>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>>> That seems awkward.
>>
>> Violates the contract's "like qemu_strtod()".
> 
> Right, I missed that. What about something like this:
> 
> int qemu_strtod_finite(const char *nptr, const char **endptr, double
> *result)
> {
>      double tmp;
>      int ret;
> 
>      ret = qemu_strtod(nptr, endptr, &tmp);
>      if (!ret && !isfinite(tmp)) {
>          if (endptr) {
>              *endptr = nptr;
>          }
>          ret = -EINVAL;
>      }
> 
>      if (ret != -EINVAL) {
>          *result = tmp;
>      }
>      return ret;
> }

With that algorithm, v3 can have:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 21:07, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> Let's provide a wrapper for strtod().
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> This changed enough from v1 that I would have dropped R-b to ensure
>>> that reviewers notice the differences.
>
> Indeed, dropping it now ;)
>
>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>   include/qemu/cutils.h |  2 ++
>>>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 67 insertions(+)
>>>>
>>>
>>>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>>>> + * on the sign, and return -ERANGE.
>>>> + *
>>>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>>>> + * sign, and return -ERANGE.
>>>
>>> The use of UTF-8 ± in one place but not both is odd.  I think we're at
>>> the point where UTF-8 comments are acceptable these days, rather than
>>> trying to keep our codebase ASCII-clean, so I don't care which way you
>>> resolve the inconsistency.
>> 
>> 217 out of 6455 git-controlled files contain non-ASCII characters.  53
>> of them are binary, and don't count.  In most text files, it's for
>> spelling names of authors properly in comments.  Ample precedence for
>> UTF-8 in comments, I'd say.
>> 
>> That said, I second Eric's call for consistency, with the slightest of
>> preferrences for plain ASCII.
>
> I'll just go with +/-. Thanks.
>
>> 
>> I spotted UTF-8 in two error messages, which might still be unadvisable:
>> 
>> hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
>> hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
>> 
>>>> +/**
>>>> + * Convert string @nptr to a finite double.
>>>> + *
>>>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>>>> + * with -EINVAL and no conversion is performed.
>>>> + */
>>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>>> +{
>>>> +    double tmp;
>>>> +    int ret;
>>>> +
>>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>>> +    if (ret) {
>>>> +        return ret;
>>>
>>> So, if we overflow, we are returning -ERANGE but with nothing stored
>>> into *result.  This is different from qemu_strtod(), where a return of
>>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>>> That seems awkward.
>> 
>> Violates the contract's "like qemu_strtod()".
>
> Right, I missed that. What about something like this:
>
> int qemu_strtod_finite(const char *nptr, const char **endptr, double
> *result)
> {
>     double tmp;
>     int ret;
>
>     ret = qemu_strtod(nptr, endptr, &tmp);
>     if (!ret && !isfinite(tmp)) {
>         if (endptr) {
>             *endptr = nptr;
>         }
>         ret = -EINVAL;
>     }
>
>     if (ret != -EINVAL) {
>         *result = tmp;
>     }
>     return ret;
> }

With these changes:
Reviewed-by: Markus Armbruster <armbru@redhat.com>