| Message ID | 1487067971-10443-9-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 14 February 2017 at 10:25, Markus Armbruster <armbru@redhat.com> wrote: > Reorder check_strtox_error() to make it obvious that we always store > through a non-null @endptr. > > Transform > > if (some error) { > error case ... > err = value for error case; > } else { > normal case ... > err = value for normal case; > } > return err; > > to > > if (some error) { > error case ... > return value for error case; > } > normal case ... > return value for normal case; > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/cutils.c | 89 ++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 45 insertions(+), 44 deletions(-) > > diff --git a/util/cutils.c b/util/cutils.c > index 7d165bc..7442d46 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -265,15 +265,20 @@ int64_t qemu_strtosz(const char *nptr, char **end) > static int check_strtox_error(const char *nptr, char *ep, > const char **endptr, int eno) > { > - if (eno == 0 && ep == nptr) { > - eno = EINVAL; > - } > - if (!endptr && *ep) { > - return -EINVAL; > - } > if (endptr) { > *endptr = ep; > } > + > + /* Turn "no conversion" into an error */ > + if (eno == 0 && ep == nptr) { > + return -EINVAL; > + } > + > + /* Fail when we're expected to consume the string, but didn't */ > + if (!endptr && *ep) { > + return -EINVAL; > + } > + > return -eno; > } Doesn't this change the semantics? Previously, for the case of (eno == 0 && ep == nptr) we would both set *endptr to ep and return -EINVAL. Now we only return -EINVAL and leave *endptr alone. The comment describing the qemu_strtol() API is a bit vague about what exactly we keep from the strtol() semantics for "no convertable characters" but I would assume that retaining "*endptr is written with the original value of nptr" is intentional. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 14 February 2017 at 10:25, Markus Armbruster <armbru@redhat.com> wrote: >> Reorder check_strtox_error() to make it obvious that we always store >> through a non-null @endptr. >> >> Transform >> >> if (some error) { >> error case ... >> err = value for error case; >> } else { >> normal case ... >> err = value for normal case; >> } >> return err; >> >> to >> >> if (some error) { >> error case ... >> return value for error case; >> } >> normal case ... >> return value for normal case; >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> util/cutils.c | 89 ++++++++++++++++++++++++++++++----------------------------- >> 1 file changed, 45 insertions(+), 44 deletions(-) >> >> diff --git a/util/cutils.c b/util/cutils.c >> index 7d165bc..7442d46 100644 >> --- a/util/cutils.c >> +++ b/util/cutils.c >> @@ -265,15 +265,20 @@ int64_t qemu_strtosz(const char *nptr, char **end) >> static int check_strtox_error(const char *nptr, char *ep, >> const char **endptr, int eno) >> { >> - if (eno == 0 && ep == nptr) { >> - eno = EINVAL; >> - } >> - if (!endptr && *ep) { >> - return -EINVAL; >> - } >> if (endptr) { >> *endptr = ep; >> } >> + >> + /* Turn "no conversion" into an error */ >> + if (eno == 0 && ep == nptr) { >> + return -EINVAL; >> + } >> + >> + /* Fail when we're expected to consume the string, but didn't */ >> + if (!endptr && *ep) { >> + return -EINVAL; >> + } >> + >> return -eno; >> } > > Doesn't this change the semantics? Previously, for the > case of (eno == 0 && ep == nptr) we would both set > *endptr to ep and return -EINVAL. Now we only return -EINVAL > and leave *endptr alone. Behavior before the patch: if (eno == 0 && ep == nptr) { eno = EINVAL; // set return value to EINVAL, but continue } if (!endptr && *ep) { return -EINVAL; // return -EINVAL without setting *endptr // correct because endptr is null } if (endptr) { *endptr = ep; // set *endptr unless endptr is null } return -eno; // *endptr got set unless endptr is null As you say, we set *endptr unless it's null. After the patch: if (endptr) { *endptr = ep; // set *endptr unless endptr is null } // no matter what happens below, *endptr got set unless endptr is null /* Turn "no conversion" into an error */ if (eno == 0 && ep == nptr) { return -EINVAL; } /* Fail when we're expected to consume the string, but didn't */ if (!endptr && *ep) { return -EINVAL; } return -eno; No change, as far as I can tell. > The comment describing the > qemu_strtol() API is a bit vague about what exactly we keep > from the strtol() semantics for "no convertable characters" > but I would assume that retaining "*endptr is written with > the original value of nptr" is intentional. I rewrite the comment PATCH 05. Relevant part: * @nptr may be null, and no conversion is performed then. * * If no conversion is performed, store @nptr in *@endptr and return * -EINVAL. I guess I should qualify "store @nptr in *@endptr" with "unless @endptr is null" for completeness. Anything else to improve?
On 14 February 2017 at 12:58, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> Doesn't this change the semantics? Previously, for the >> case of (eno == 0 && ep == nptr) we would both set >> *endptr to ep and return -EINVAL. Now we only return -EINVAL >> and leave *endptr alone. > No change, as far as I can tell. Ah, you're right, and I misread the diff. Sorry. thanks -- PMM
On 02/14/2017 04:25 AM, Markus Armbruster wrote: > Reorder check_strtox_error() to make it obvious that we always store > through a non-null @endptr. > > Transform > > if (some error) { > error case ... > err = value for error case; > } else { > normal case ... > err = value for normal case; > } > return err; > > to > > if (some error) { > error case ... > return value for error case; > } > normal case ... > return value for normal case; > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/cutils.c | 89 ++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 45 insertions(+), 44 deletions(-) No real net change in lines, but I agree that it is more legible. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/util/cutils.c b/util/cutils.c index 7d165bc..7442d46 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -265,15 +265,20 @@ int64_t qemu_strtosz(const char *nptr, char **end) static int check_strtox_error(const char *nptr, char *ep, const char **endptr, int eno) { - if (eno == 0 && ep == nptr) { - eno = EINVAL; - } - if (!endptr && *ep) { - return -EINVAL; - } if (endptr) { *endptr = ep; } + + /* Turn "no conversion" into an error */ + if (eno == 0 && ep == nptr) { + return -EINVAL; + } + + /* Fail when we're expected to consume the string, but didn't */ + if (!endptr && *ep) { + return -EINVAL; + } + return -eno; } @@ -305,18 +310,17 @@ int qemu_strtol(const char *nptr, const char **endptr, int base, long *result) { char *ep; - int err = 0; + if (!nptr) { if (endptr) { *endptr = nptr; } - err = -EINVAL; - } else { - errno = 0; - *result = strtol(nptr, &ep, base); - err = check_strtox_error(nptr, ep, endptr, errno); + return -EINVAL; } - return err; + + errno = 0; + *result = strtol(nptr, &ep, base); + return check_strtox_error(nptr, ep, endptr, errno); } /** @@ -348,22 +352,21 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base, unsigned long *result) { char *ep; - int err = 0; + if (!nptr) { if (endptr) { *endptr = nptr; } - err = -EINVAL; - } else { - errno = 0; - *result = strtoul(nptr, &ep, base); - /* Windows returns 1 for negative out-of-range values. */ - if (errno == ERANGE) { - *result = -1; - } - err = check_strtox_error(nptr, ep, endptr, errno); + return -EINVAL; } - return err; + + errno = 0; + *result = strtoul(nptr, &ep, base); + /* Windows returns 1 for negative out-of-range values. */ + if (errno == ERANGE) { + *result = -1; + } + return check_strtox_error(nptr, ep, endptr, errno); } /** @@ -376,19 +379,18 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base, int64_t *result) { char *ep; - int err = 0; + if (!nptr) { if (endptr) { *endptr = nptr; } - err = -EINVAL; - } else { - errno = 0; - /* FIXME This assumes int64_t is long long */ - *result = strtoll(nptr, &ep, base); - err = check_strtox_error(nptr, ep, endptr, errno); + return -EINVAL; } - return err; + + errno = 0; + /* FIXME This assumes int64_t is long long */ + *result = strtoll(nptr, &ep, base); + return check_strtox_error(nptr, ep, endptr, errno); } /** @@ -400,23 +402,22 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base, uint64_t *result) { char *ep; - int err = 0; + if (!nptr) { if (endptr) { *endptr = nptr; } - err = -EINVAL; - } else { - errno = 0; - /* FIXME This assumes uint64_t is unsigned long long */ - *result = strtoull(nptr, &ep, base); - /* Windows returns 1 for negative out-of-range values. */ - if (errno == ERANGE) { - *result = -1; - } - err = check_strtox_error(nptr, ep, endptr, errno); + return -EINVAL; } - return err; + + errno = 0; + /* FIXME This assumes uint64_t is unsigned long long */ + *result = strtoull(nptr, &ep, base); + /* Windows returns 1 for negative out-of-range values. */ + if (errno == ERANGE) { + *result = -1; + } + return check_strtox_error(nptr, ep, endptr, errno); } /**
Reorder check_strtox_error() to make it obvious that we always store through a non-null @endptr. Transform if (some error) { error case ... err = value for error case; } else { normal case ... err = value for normal case; } return err; to if (some error) { error case ... return value for error case; } normal case ... return value for normal case; Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/cutils.c | 89 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 45 insertions(+), 44 deletions(-)