1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 00/24] QemuOpts util/cutils: Fix and clean up number conversions
  4. View patch

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

Markus Armbruster posted 24 patches 8 years, 11 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit
Posted by Markus Armbruster 8 years, 11 months ago 
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;
 }
 
@@ -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);
 }
 
 /**
-- 
2.7.4
Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit
Posted by Peter Maydell 8 years, 11 months ago 
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
Re: [Qemu-devel] [PATCH 08/24] util/cutils: Clean up control flow around qemu_strtol() a bit
Posted by Eric Blake 8 years, 11 months ago 
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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.