|
From: | Eric Blake |
Subject: | Re: [Qemu-devel] [PATCH v1 2/9] cutils: use qemu_strtod_finite() in do_strtosz() |
Date: | Thu, 15 Nov 2018 08:36:19 -0600 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
On 11/15/18 8:04 AM, David Hildenbrand wrote:
Let's use the new function. In order to do so, we have to convert all users of qemu_strtosz*() to pass a "const char **end" ptr. We will now also reject "inf" properly. Signed-off-by: David Hildenbrand <address@hidden> --- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 14 +++++++------- util/cutils.c | 14 ++++++-------- 4 files changed, 17 insertions(+), 19 deletions(-)
+++ b/tests/test-cutils.c @@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void) static void test_qemu_strtosz_simple(void) { const char *str; - char *endptr = NULL; + const char *endptr = NULL;
...
diff --git a/util/cutils.c b/util/cutils.c
Conversion of call sites is good (in fact, you could drop the ' = NULL' initialization, since do_strtosz() guarantees it is set to something sane even on error). But where's the added test coverage of rejecting "inf" as a size?
index 7868a683e8..dd61fb4558 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit) * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on * other error. */ -static int do_strtosz(const char *nptr, char **end, +static int do_strtosz(const char *nptr, const char **end, const char default_suffix, int64_t unit, uint64_t *result) { int retval; - char *endptr; + const char *endptr; unsigned char c; int mul_required = 0; double val, mul, integral, fraction;- errno = 0;- val = strtod(nptr, &endptr); - if (isnan(val) || endptr == nptr || errno != 0) { + if (qemu_strtod_finite(nptr, &endptr, &val)) { retval = -EINVAL;
This slams -ERANGE failure into -EINVAL. Do we care? Or would it have been better to just do:
retval = qemu_strtod_finite(...); if (retval) { goto out;
goto out; } @@ -259,17 +257,17 @@ out:
More context: out: if (end) { *end = endptr; } else if (*endptr) { retval = -EINVAL; }
return retval; }
Everything else looks okay.Hmm - while touching this, is it worth making mul_required be a bool, to match its use?
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |