Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Markus Armbruster <armbru@redhat.com>]

Still not entirely happy, but maybe we can commit it as is, and fix it
up later.

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> This version lets strtod() do the actual parsing instead of relying on
> strspn, as well as catches NaN input.

This is information on what changed since v5, and as such it doesn't
belong into the commit message.

> The following suffixes are supported:
> B/b = bytes
> K/k = KB
> M/m = MB
> G/g = GB
> T/t = TB
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Would be nice if commit message documented that this affects -numa and
-m.  In particular that they now accept more suffixes than before.

> ---
>  cutils.c      |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  vl.c          |   31 +++++++---------------
>  3 files changed, 90 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..8e79d92 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -23,6 +23,7 @@
>   */
>  #include "qemu-common.h"
>  #include "host-utils.h"
> +#include <math.h>
>  
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
> @@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
>  }
>  #endif
>  
> +/*
> + * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
> + * M/m for MB, G/g for GB or T/t for TB. Default without any postfix
> + * is MB. End pointer will be returned in *end, if end is valid.

Nitpick: There are plenty of invalid pointers we'll happily attempt to
use.  "unless end is null" would be more precise.

> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> +    ssize_t retval = -1;
> +    int64_t tmpval;
> +    char *endptr, c;
> +    int mul_required = 0;
> +    double val, mul, integral, fraction;
> +
> +    errno = 0;
> +    val = strtod(nptr, &endptr);
> +    if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
> +        val == HUGE_VAL) {

ISO C permits implementations supporting infinities to make HUGE_VAL
*not* +inf.  So this may not catch +inf.  val >= HUGE_VAL would.

But since we have to catch val * mul out of range further down anyway,
the check for HUGE_VAL may be redundant here.

> +        goto fail;
> +    }
> +    integral = modf(val, &fraction);
> +    if (integral != 0) {
> +        mul_required = 1;
> +    }
> +    /*
> +     * Any whitespace character is fine for terminating the number,
> +     * otherwise check that the suffix is just one character.
> +     */
> +    c = *endptr++;
> +    if (isspace(c) || c == '\0') {
> +        c = 0;
> +    } else if (!isspace(*endptr) && *endptr != 0) {
> +        goto fail;
> +    }

I'm not happy with this check.

If the caller needs a complete string consumed, then this check is
insufficient, because it doesn't catch trailing garbage as long as it
starts with whitespace.  The caller still needs to check !*endptr.

If the caller needs to continue parsing after the value, and expects
anything but whitespace there, it has to copy the value first.  Only
easy if the value is followed by some delimiter that can't occur in the
value.  Example: parse a size value from something of them form
name=value,name=value...  Need to copy up to the next comma or end of
string.

The check complicates the second case without really helping the first
case.

Nevertheless, it's good enough for the uses in this patch series, so I'm
not insisting on getting this changed now.

> +    switch (c) {
> +    case 'B':
> +    case 'b':
> +        mul = 1;
> +        if (mul_required) {
> +            goto fail;
> +        }
> +        break;
> +    case 'K':
> +    case 'k':
> +        mul = 1 << 10;
> +        break;
> +    case 0:
> +        if (mul_required) {
> +            goto fail;
> +        }
> +    case 'M':
> +    case 'm':
> +        mul = 1ULL << 20;
> +        break;
> +    case 'G':
> +    case 'g':
> +        mul = 1ULL << 30;
> +        break;
> +    case 'T':
> +    case 't':
> +        mul = 1ULL << 40;
> +        break;
> +    default:
> +        goto fail;
> +    }
> +    tmpval = (val * mul);
> +    if (tmpval >= ~(size_t)0) {
> +        goto fail;

val * mul may exceed the range of int64_t tmpval, and then the
assignment has undefined behavior.  Obvious way to avoid that:

    if (val * mul >= ~(size_t)0) {
        goto fail;
    }
    retval = val * mul;

> +    }
> +    retval = tmpval;
> +
> +    if (end) {
> +        *end = endptr;
> +    }
> +
> +fail:
> +    return retval;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>          if (get_param_value(option, 128, "mem", optarg) == 0) {
>              node_mem[nodenr] = 0;
>          } else {
> -            value = strtoull(option, &endptr, 0);
> -            switch (*endptr) {
> -            case 0: case 'M': case 'm':
> -                value <<= 20;
> -                break;
> -            case 'G': case 'g':
> -                value <<= 30;
> -                break;
> +            ssize_t sval;
> +            sval = strtosz(option, NULL);
> +            if (sval < 0) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> +                exit(1);
>              }
> -            node_mem[nodenr] = value;
> +            node_mem[nodenr] = sval;
>          }
>          if (get_param_value(option, 128, "cpus", optarg) == 0) {
>              node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m: {
> -                uint64_t value;
> -                char *ptr;
> +                ssize_t value;
>  
> -                value = strtoul(optarg, &ptr, 10);
> -                switch (*ptr) {
> -                case 0: case 'M': case 'm':
> -                    value <<= 20;
> -                    break;
> -                case 'G': case 'g':
> -                    value <<= 30;
> -                    break;
> -                default:
> +                value = strtosz(optarg, NULL);
> +                if (value < 0) {
>                      fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
>                      exit(1);
>                  }