Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit

[Peter Maydell <peter.maydell@linaro.org>]

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