Eric Blake <ebl...@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 <da...@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 <ebl...@redhat.com> Thanks for your analysis, Eric. With the typo fixes: Reviewed-by: Markus Armbruster <arm...@redhat.com>