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

[Markus Armbruster <armbru@redhat.com>]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  vl.c          |   26 +++++++-------------------
>  3 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 036ae3c..dabbed4 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -251,3 +251,42 @@ int fcntl_setfl(int fd, int flag)
>  }
>  #endif
>  
> +/*
> + * Convert string to bytes, allowing either 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.
> + * Return 0 on error.

Funny error value.  Perhaps a more conventional choice would be
(uint64_t)-1.

> + */
> +uint64_t strtobytes(const char *nptr, char **end)

I don't like the name.  It suggests the function parses a number of
bytes from the string.

size_t strtoz() would make sense (think printf/scanf conversion
specifier "%zd").  But only if you use size_t.  If not, then perhaps
strtosize() or strtocount().

> +{
> +    uint64_t value;
> +    char *endptr;
> +
> +    value = strtoll(nptr, &endptr, 0);

Unsigned variable, signed parsing function.  What about making the two
consistent?  unsigned long long value = stroull(...)

Unless you change the return type as well, you then have to worry about
uint64_t having a different width than unsigned long long.  Cleanest
solution would be parsing into uintmax_t with strtoumax(), then checked
conversion to uint64_t.

Next, this seems to accept blank input.  Intentional?  Works out because
stroull() returns zero then, which happens to be your error value.

Furthermore, you don't detect overflow here.  If you want to do that,
set errno = 0 before, and check it afterwards.

Here's the idiomatic way to use strtol() & friends correctly:

    errno = 0;
    l = strtol(str, end, 10);
    if (*end == str || errno != 0)
        FAIL();

> +    switch (*endptr++) {
> +    case 'K':
> +    case 'k':
> +        value <<= 10;
> +        break;
> +    case 0:
> +    case 'M':
> +    case 'm':
> +        value <<= 20;
> +        break;
> +    case 'G':
> +    case 'g':
> +        value <<= 30;
> +        break;
> +    case 'T':
> +    case 't':
> +        value <<= 40;
> +        break;
> +    default:
> +        value = 0;
> +    }
> +
> +    if (end)
> +        *end = endptr;
> +
> +    return value;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index dfd3dc0..01d7dcb 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -137,6 +137,7 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +uint64_t strtobytes(const char *nptr, char **end);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index 3f45aa9..0cdd94a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,14 +734,10 @@ 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;
> +            value = strtobytes(option, NULL);
> +            if (!value) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> +                exit(1);
>              }
>              node_mem[nodenr] = value;
>          }

You add proper error handling where there was none before.  That's good,
but the commit message doesn't mention it.  Separate patch?

> @@ -2166,17 +2162,9 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_m: {
>                  uint64_t value;
> -                char *ptr;
>  
> -                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 = strtobytes(optarg, NULL);
> +                if (!value) {
>                      fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
>                      exit(1);
>                  }