Eric Blake <ebl...@redhat.com> writes: > On 02/14/2017 04:26 AM, Markus Armbruster wrote: >> Change the qemu_strtosz() & friends to return -EINVAL when @endptr is >> null and the conversion doesn't consume the string completely. >> Matches how qemu_strtol() & friends work. >> >> Only test_qemu_strtosz_simple() passes a null @endptr. No functional >> change there, because its conversion consumes the string. >> >> Simplify callers that use @endptr only to fail when it doesn't point >> to '\0' to pass a null @endptr instead. >> >> Cc: Dr. David Alan Gilbert <dgilb...@redhat.com> >> Cc: Eduardo Habkost <ehabk...@redhat.com> (maintainer:X86) >> Cc: Kevin Wolf <kw...@redhat.com> (supporter:Block layer core) >> Cc: Max Reitz <mre...@redhat.com> (supporter:Block layer core) >> Cc: qemu-bl...@nongnu.org (open list:Block layer core) >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> hmp.c | 6 ++---- >> hw/misc/ivshmem.c | 6 ++---- >> qapi/opts-visitor.c | 5 ++--- >> qemu-img.c | 7 +------ >> qemu-io-cmds.c | 7 +------ >> target/i386/cpu.c | 5 ++--- >> tests/test-cutils.c | 6 ++++++ >> util/cutils.c | 14 +++++++++----- >> 8 files changed, 25 insertions(+), 31 deletions(-) > > Nice cleanup. > > Reviewed-by: Eric Blake <ebl...@redhat.com> > > >> +++ b/qemu-img.c >> @@ -370,14 +370,9 @@ static int add_old_style_options(const char *fmt, >> QemuOpts *opts, >> >> static int64_t cvtnum(const char *s) >> { > >> +++ b/qemu-io-cmds.c >> @@ -137,14 +137,9 @@ static char **breakline(char *input, int *count) >> >> static int64_t cvtnum(const char *s) >> { >> - char *end; > > Why do we reimplement cvtnum() as copied static functions instead of > exporting it? But that would be a separate cleanup (perhaps squashed > into 20/24, where you use cvtnum in qemu-img).
At the end of this series, the two cvtnum() look like this: static int64_t cvtnum(const char *s) { int err; uint64_t value; err = qemu_strtosz(s, NULL, &value); if (err < 0) { return err; } if (value > INT64_MAX) { return -ERANGE; } return value; } Does two things: limit value range to 0..INT64_MAX, merge error into value. Perhaps their callers should simply use qemu_strtosz() directly. I chose not to go there to limit churn in this series. >> @@ -217,7 +217,8 @@ static int64_t do_strtosz(const char *nptr, char **end, >> errno = 0; >> val = strtod(nptr, &endptr); >> if (isnan(val) || endptr == nptr || errno != 0) { > > Hmm - we explicitly reject "NaN", but not "infinity". But when strtod() > accepts infinity, ... > >> - goto fail; >> + retval = -EINVAL; >> + goto out; >> } >> fraction = modf(val, &integral); > > then modf() returns 0 with integral left at infinity... > >> if (fraction != 0) { >> @@ -232,17 +233,20 @@ static int64_t do_strtosz(const char *nptr, char **end, >> assert(mul >= 0); >> } >> if (mul == 1 && mul_required) { >> - goto fail; >> + retval = -EINVAL; >> + goto out; >> } >> if ((val * mul >= INT64_MAX) || val < 0) { > > ...and the multiply exceeds INT64_MAX, so we still end up rejecting it > (with ERANGE instead of EINVAL). Weird way but seems to work, and is > pre-existing, so not this patch's problem. Yes. We could easily check !isfinite() rather than isnan(), but then we'd get -EINVAL rather than -ERANGE for infinities. Matter of taste, I guess.