qemu_strtosz() & friends reject NaNs, but happily accept inifities.
They shouldn't. Fix that.
The fix makes use of qemu_strtod_finite(). To avoid ugly casts,
change the @end parameter of qemu_strtosz() & friends from char **
to const char **.
Also, add two test cases, testing that "inf" and "NaN" are properly
rejected.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/qemu/cutils.h | 6 +++---
monitor.c | 2 +-
tests/test-cutils.c | 24 +++++++++++++++++-------
util/cutils.c | 16 +++++++---------
4 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 756b41c193..d2dad3057c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -153,9 +153,9 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
int base);
int parse_uint_full(const char *s, unsigned long long *value, int base);
-int qemu_strtosz(const char *nptr, char **end, uint64_t *result);
-int qemu_strtosz_MiB(const char *nptr, char **end, uint64_t *result);
-int qemu_strtosz_metric(const char *nptr, char **end, uint64_t *result);
+int qemu_strtosz(const char *nptr, const char **end, uint64_t *result);
+int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result);
+int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
/* used to print char* safely */
#define STR_OR_NULL(str) ((str) ? (str) : "null")
diff --git a/monitor.c b/monitor.c
index d39390c2f2..ee9893c785 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3231,7 +3231,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
int ret;
uint64_t val;
- char *end;
+ const char *end;
while (qemu_isspace(*p)) {
p++;
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index d85c3e0f6d..1aa8351520 100644
--- a/tests/test-cutils.c
+++ 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;
int err;
uint64_t res = 0xbaadf00d;
@@ -2017,7 +2017,7 @@ static void test_qemu_strtosz_units(void)
const char *p = "1P";
const char *e = "1E";
int err;
- char *endptr = NULL;
+ const char *endptr;
uint64_t res = 0xbaadf00d;
/* default is M */
@@ -2066,7 +2066,7 @@ static void test_qemu_strtosz_float(void)
{
const char *str = "12.345M";
int err;
- char *endptr = NULL;
+ const char *endptr;
uint64_t res = 0xbaadf00d;
err = qemu_strtosz(str, &endptr, &res);
@@ -2078,7 +2078,7 @@ static void test_qemu_strtosz_float(void)
static void test_qemu_strtosz_invalid(void)
{
const char *str;
- char *endptr = NULL;
+ const char *endptr;
int err;
uint64_t res = 0xbaadf00d;
@@ -2096,12 +2096,22 @@ static void test_qemu_strtosz_invalid(void)
err = qemu_strtosz(str, &endptr, &res);
g_assert_cmpint(err, ==, -EINVAL);
g_assert(endptr == str);
+
+ str = "inf";
+ err = qemu_strtosz(str, &endptr, &res);
+ g_assert_cmpint(err, ==, -EINVAL);
+ g_assert(endptr == str);
+
+ str = "NaN";
+ err = qemu_strtosz(str, &endptr, &res);
+ g_assert_cmpint(err, ==, -EINVAL);
+ g_assert(endptr == str);
}
static void test_qemu_strtosz_trailing(void)
{
const char *str;
- char *endptr = NULL;
+ const char *endptr;
int err;
uint64_t res = 0xbaadf00d;
@@ -2126,7 +2136,7 @@ static void test_qemu_strtosz_trailing(void)
static void test_qemu_strtosz_erange(void)
{
const char *str;
- char *endptr = NULL;
+ const char *endptr;
int err;
uint64_t res = 0xbaadf00d;
@@ -2160,7 +2170,7 @@ static void test_qemu_strtosz_metric(void)
{
const char *str = "12345k";
int err;
- char *endptr = NULL;
+ const char *endptr;
uint64_t res = 0xbaadf00d;
err = qemu_strtosz_metric(str, &endptr, &res);
diff --git a/util/cutils.c b/util/cutils.c
index c965dbfcad..98732ff5b2 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -206,20 +206,18 @@ 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) {
- retval = -EINVAL;
+ retval = qemu_strtod_finite(nptr, &endptr, &val);
+ if (retval) {
goto out;
}
fraction = modf(val, &integral);
@@ -259,17 +257,17 @@ out:
return retval;
}
-int qemu_strtosz(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
{
return do_strtosz(nptr, end, 'B', 1024, result);
}
-int qemu_strtosz_MiB(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
{
return do_strtosz(nptr, end, 'M', 1024, result);
}
-int qemu_strtosz_metric(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
{
return do_strtosz(nptr, end, 'B', 1000, result);
}
--
2.17.2
On 11/20/18 3:25 AM, David Hildenbrand wrote:
> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
s/inifities/infinities/
> They shouldn't. Fix that.
>
> The fix makes use of qemu_strtod_finite(). To avoid ugly casts,
> change the @end parameter of qemu_strtosz() & friends from char **
> to const char **.
>
> Also, add two test cases, testing that "inf" and "NaN" are properly
> rejected.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> include/qemu/cutils.h | 6 +++---
> monitor.c | 2 +-
> tests/test-cutils.c | 24 +++++++++++++++++-------
> util/cutils.c | 16 +++++++---------
> 4 files changed, 28 insertions(+), 20 deletions(-)
>
> +++ b/util/cutils.c
> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
Pre-existing, but since you're touching this area: the second 'Return'
is unusual capitalization for being mid-sentence. You could even
s/Return/of/
> * 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) {
> - retval = -EINVAL;
> + retval = qemu_strtod_finite(nptr, &endptr, &val);
> + if (retval) {
> goto out;
Here, retval can be -EINVAL (for failure to parse, or encountering "inf"
or "NaN") or -ERANGE (overflow, underflow)...
> }
> fraction = modf(val, &integral);
> @@ -259,17 +257,17 @@ out:
out:
if (end) {
*end = endptr;
} else if (*endptr) {
retval = -EINVAL;
}
> return retval;
...if the failure was -EINVAL due to trailing garbage or empty string,
nothing changes. If the failure was -EINVAL due to "inf", and the user
passed in 'end', then 'end' now points to the beginning of "inf" instead
of the end (probably okay). If the failure was -EINVAL due to "inf" and
the user gave NULL for 'end', then we slam retval back to -EINVAL (no
change). If the failure was -ERANGE, then there is no trailing garbage,
so *endptr had better be NULL, and we still fail with -ERANGE. Any
other way to reach the out label is unchanged from earlier logic.
It's some hairy code to think about, but I can't find anything wrong
with it. Typo fixes are minor, so
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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:
>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>
> s/inifities/infinities/
>
>> They shouldn't. Fix that.
>>
>> The fix makes use of qemu_strtod_finite(). To avoid ugly casts,
>> change the @end parameter of qemu_strtosz() & friends from char **
>> to const char **.
>>
>> Also, add two test cases, testing that "inf" and "NaN" are properly
>> rejected.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> include/qemu/cutils.h | 6 +++---
>> monitor.c | 2 +-
>> tests/test-cutils.c | 24 +++++++++++++++++-------
>> util/cutils.c | 16 +++++++---------
>> 4 files changed, 28 insertions(+), 20 deletions(-)
>>
>
>> +++ b/util/cutils.c
>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>
> Pre-existing, but since you're touching this area: the second 'Return'
> is unusual capitalization for being mid-sentence. You could even
> s/Return/of/
"of"?
>
>> * 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) {
>> - retval = -EINVAL;
>> + retval = qemu_strtod_finite(nptr, &endptr, &val);
>> + if (retval) {
>> goto out;
>
> Here, retval can be -EINVAL (for failure to parse, or encountering
> "inf" or "NaN") or -ERANGE (overflow, underflow)...
>
>> }
>> fraction = modf(val, &integral);
>> @@ -259,17 +257,17 @@ out:
>
> out:
> if (end) {
> *end = endptr;
> } else if (*endptr) {
> retval = -EINVAL;
> }
>
>> return retval;
>
> ...if the failure was -EINVAL due to trailing garbage or empty string,
> nothing changes. If the failure was -EINVAL due to "inf", and the user
> passed in 'end', then 'end' now points to the beginning of "inf"
> instead of the end (probably okay). If the failure was -EINVAL due to
> "inf" and the user gave NULL for 'end', then we slam retval back to
> -EINVAL (no change). If the failure was -ERANGE, then there is no
> trailing garbage, so *endptr had better be NULL, and we still fail
> with -ERANGE. Any other way to reach the out label is unchanged from
> earlier logic.
>
> It's some hairy code to think about, but I can't find anything wrong
> with it. Typo fixes are minor, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks for your analysis, Eric.
With the typo fixes:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 11/20/18 2:31 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>> qemu_strtosz() & friends reject NaNs, but happily accept inifities. >> >> s/inifities/infinities/ >> >>> They shouldn't. Fix that. >>> >>> The fix makes use of qemu_strtod_finite(). To avoid ugly casts, >>> change the @end parameter of qemu_strtosz() & friends from char ** >>> to const char **. >>> >>> Also, add two test cases, testing that "inf" and "NaN" are properly >>> rejected. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> include/qemu/cutils.h | 6 +++--- >>> monitor.c | 2 +- >>> tests/test-cutils.c | 24 +++++++++++++++++------- >>> util/cutils.c | 16 +++++++--------- >>> 4 files changed, 28 insertions(+), 20 deletions(-) >>> >> >>> +++ b/util/cutils.c >>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit) >>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on >> >> Pre-existing, but since you're touching this area: the second 'Return' >> is unusual capitalization for being mid-sentence. You could even >> s/Return/of/ > > "of"? "or" (ouch - wrong time for my fingers to be slipping on the keyboard) >> It's some hairy code to think about, but I can't find anything wrong >> with it. Typo fixes are minor, so >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Thanks for your analysis, Eric. > > With the typo fixes: Including the fix of my attempt at a typo fix :) > Reviewed-by: Markus Armbruster <armbru@redhat.com> > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 20.11.18 21:41, Eric Blake wrote: > On 11/20/18 2:31 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities. >>> >>> s/inifities/infinities/ >>> >>>> They shouldn't. Fix that. >>>> >>>> The fix makes use of qemu_strtod_finite(). To avoid ugly casts, >>>> change the @end parameter of qemu_strtosz() & friends from char ** >>>> to const char **. >>>> >>>> Also, add two test cases, testing that "inf" and "NaN" are properly >>>> rejected. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> include/qemu/cutils.h | 6 +++--- >>>> monitor.c | 2 +- >>>> tests/test-cutils.c | 24 +++++++++++++++++------- >>>> util/cutils.c | 16 +++++++--------- >>>> 4 files changed, 28 insertions(+), 20 deletions(-) >>>> >>> >>>> +++ b/util/cutils.c >>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit) >>>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on >>> >>> Pre-existing, but since you're touching this area: the second 'Return' >>> is unusual capitalization for being mid-sentence. You could even >>> s/Return/of/ >> >> "of"? > > "or" (ouch - wrong time for my fingers to be slipping on the keyboard) Shouldn't that be "and" and s/Return/Returns/ "Returns -ERANGE on overflow and -EINVAL on other errors". I can include that fixup (whetever version you guys prefer) > > >>> It's some hairy code to think about, but I can't find anything wrong >>> with it. Typo fixes are minor, so >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Thanks for your analysis, Eric. >> >> With the typo fixes: > > Including the fix of my attempt at a typo fix :) > >> Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks! -- Thanks, David / dhildenb
David Hildenbrand <david@redhat.com> writes: > On 20.11.18 21:41, Eric Blake wrote: >> On 11/20/18 2:31 PM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities. >>>> >>>> s/inifities/infinities/ >>>> >>>>> They shouldn't. Fix that. >>>>> >>>>> The fix makes use of qemu_strtod_finite(). To avoid ugly casts, >>>>> change the @end parameter of qemu_strtosz() & friends from char ** >>>>> to const char **. >>>>> >>>>> Also, add two test cases, testing that "inf" and "NaN" are properly >>>>> rejected. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> include/qemu/cutils.h | 6 +++--- >>>>> monitor.c | 2 +- >>>>> tests/test-cutils.c | 24 +++++++++++++++++------- >>>>> util/cutils.c | 16 +++++++--------- >>>>> 4 files changed, 28 insertions(+), 20 deletions(-) >>>>> >>>> >>>>> +++ b/util/cutils.c >>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit) >>>>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on >>>> >>>> Pre-existing, but since you're touching this area: the second 'Return' >>>> is unusual capitalization for being mid-sentence. You could even >>>> s/Return/of/ >>> >>> "of"? >> >> "or" (ouch - wrong time for my fingers to be slipping on the keyboard) > > Shouldn't that be "and" and s/Return/Returns/ > > > "Returns -ERANGE on overflow and -EINVAL on other errors". I prefer imperative mood for function contracts: "Convert string to bytes", "Return -ERANGE on overflow", and so forth. Other than that, I like your phrasing. I'd put a comma before "and", though. > I can include that fixup (whetever version you guys prefer) > >> >> >>>> It's some hairy code to think about, but I can't find anything wrong >>>> with it. Typo fixes are minor, so >>>> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>> Thanks for your analysis, Eric. >>> >>> With the typo fixes: >> >> Including the fix of my attempt at a typo fix :) >> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Thanks!
On 11/21/18 4:44 AM, David Hildenbrand wrote: >>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit) >>>>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on >>>> >>>> Pre-existing, but since you're touching this area: the second 'Return' >>>> is unusual capitalization for being mid-sentence. You could even >>>> s/Return/of/ >>> >>> "of"? >> >> "or" (ouch - wrong time for my fingers to be slipping on the keyboard) > > Shouldn't that be "and" and s/Return/Returns/ > > > "Returns -ERANGE on overflow and -EINVAL on other errors". > > I can include that fixup (whetever version you guys prefer) I was thinking: Return -ERANGE on overflow, or -EINVAL on other errors. 'Return', not 'Returns', because of imperative mood. The choice of 'and' vs. 'or' is less of a sticking point; both sound fine to my native ear, especially since the word 'other' makes it apparent that you won't have both overflow and a conversion error at the same time (my initial choice of 'or' rather than 'and' was solely because you can't have two return values at once; but using 'and' seems okay at implying a sense of prioritization where overflow trumps other detected errors). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.