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 <david@redhat.com>
---
include/qemu/cutils.h | 6 +++---
monitor.c | 2 +-
tests/test-cutils.c | 14 +++++++-------
util/cutils.c | 14 ++++++--------
4 files changed, 17 insertions(+), 19 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..e21dcbf12b 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 = NULL;
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 = NULL;
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 = NULL;
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 = NULL;
int err;
uint64_t res = 0xbaadf00d;
@@ -2101,7 +2101,7 @@ static void test_qemu_strtosz_invalid(void)
static void test_qemu_strtosz_trailing(void)
{
const char *str;
- char *endptr = NULL;
+ const char *endptr = NULL;
int err;
uint64_t res = 0xbaadf00d;
@@ -2126,7 +2126,7 @@ static void test_qemu_strtosz_trailing(void)
static void test_qemu_strtosz_erange(void)
{
const char *str;
- char *endptr = NULL;
+ const char *endptr = NULL;
int err;
uint64_t res = 0xbaadf00d;
@@ -2160,7 +2160,7 @@ static void test_qemu_strtosz_metric(void)
{
const char *str = "12345k";
int err;
- char *endptr = NULL;
+ const char *endptr = NULL;
uint64_t res = 0xbaadf00d;
err = qemu_strtosz_metric(str, &endptr, &res);
diff --git a/util/cutils.c b/util/cutils.c
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;
goto out;
}
@@ -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/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 <david@redhat.com>
> ---
> 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
Eric Blake <eblake@redhat.com> writes:
> 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 shouldn't have qemu_strtol() "improve" on strtol() by splicing in
another const. Since we did, we havr to do the same for qemu_strtod().
>>
>> We will now also reject "inf" properly.
>>
Ah, so this is a bug fix! The commit message's title should tell.
Suggest something like:
cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
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 **.
I'd split the patch into just the fix (with the ugly cast) and the
parameter type change (getting rid of the ugly cast), but I'm a
compulsive patch splitter. Up to you.
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> 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).
Yes.
> But where's the added test coverage of
> rejecting "inf" as a size?
Let's stick it into test_qemu_strtosz_invalid().
>
>> 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;
If distinguishing between ERANGE and EINVAL makes sense for
qemu_strtod_finite(), it probably makes sense for qemu_strtod(), too.
>> 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?
Touches no line containing mul_required, so I wouldn't.
On 15.11.18 17:41, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> 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 shouldn't have qemu_strtol() "improve" on strtol() by splicing in
> another const. Since we did, we havr to do the same for qemu_strtod().
>
>>>
>>> We will now also reject "inf" properly.
>>>
>
> Ah, so this is a bug fix! The commit message's title should tell.
> Suggest something like:
>
> cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
>
> 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 **.
>
> I'd split the patch into just the fix (with the ugly cast) and the
> parameter type change (getting rid of the ugly cast), but I'm a
> compulsive patch splitter. Up to you.
I'll leave it in one patch for now. (and will also add two test cases
for "inf" and "NaN"). If this patch grows even bigger, it makes sense to
split.
Commit message updated.
>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> 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).
>
> Yes.
>
Dropped.
>> But where's the added test coverage of
>> rejecting "inf" as a size?
>
> Let's stick it into test_qemu_strtosz_invalid().
Done.
@@ -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);
}
>
>>
>>> 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;
>
> If distinguishing between ERANGE and EINVAL makes sense for
> qemu_strtod_finite(), it probably makes sense for qemu_strtod(), too.
Yes, will do it like that.
>
>>> 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?
>
> Touches no line containing mul_required, so I wouldn't.
>
Alright, thanks to the both of you.
--
Thanks,
David / dhildenb
© 2016 - 2025 Red Hat, Inc.