Re: [PATCH v16 03/14] util/cutils: refactor do_strtosz() to support suffixes list

[Philippe Mathieu-Daudé]

Cc'ing Markus & Stefan.

On 11/15/19 8:53 AM, Tao Xu wrote:
> Add do_strtomul() to convert string according to different suffixes.
>
> Reviewed-by: Eduardo Habkost <address@hidden>
> Signed-off-by: Tao Xu <address@hidden>
> ---
>
> No changes in v16.
>
> Changes in v15:
>      - Add a new patch to refactor do_strtosz() (Eduardo)
> ---
>   util/cutils.c | 72 ++++++++++++++++++++++++++++++---------------------
>   1 file changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/util/cutils.c b/util/cutils.c
> index d94a468954..ffef92338a 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -181,41 +181,37 @@ int fcntl_setfl(int fd, int flag)
>   }
>   #endif
>   
> -static int64_t suffix_mul(char suffix, int64_t unit)
> +static int64_t suffix_mul(const char *suffixes[], int num_suffix,
> +                          const char *endptr, int *offset, int64_t unit)
>   {
> -    switch (qemu_toupper(suffix)) {
> -    case 'B':
> -        return 1;
> -    case 'K':
> -        return unit;
> -    case 'M':
> -        return unit * unit;
> -    case 'G':
> -        return unit * unit * unit;
> -    case 'T':
> -        return unit * unit * unit * unit;
> -    case 'P':
> -        return unit * unit * unit * unit * unit;
> -    case 'E':
> -        return unit * unit * unit * unit * unit * unit;
> +    int i, suffix_len;
> +    int64_t mul = 1;
> +
> +    for (i = 0; i < num_suffix; i++) {
> +        suffix_len = strlen(suffixes[i]);
> +        if (g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {
> +            *offset = suffix_len;

So now we can parse "8kB" and "8Kb", and this might be confusing when 
parsing bit units.

  https://en.wikipedia.org/wiki/Kilobyte#Definitions_and_usage:

  IEC 80000-13 standard uses the term 'byte' to mean
  eight bits (1 B = 8 bit).

At some point we'll need to add the IEC suffix parsing to this function.

  https://en.wikipedia.org/wiki/Kibibyte#Definition

Meanwhile, can you keep it to upper case suffix only?

> +            return mul;
> +    }
> +        mul *= unit;
>       }
> +
>       return -1;
>   }
>   
>   /*
> - * 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. End pointer will be returned
> - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
> - * other error.
> + * Convert string according to different suffixes. End pointer will be returned
> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other 
> error.
>    */
> -static int do_strtosz(const char *nptr, const char **end,
> -                      const char default_suffix, int64_t unit,
> +static int do_strtomul(const char *nptr, const char **end,
> +                       const char *suffixes[], int num_suffix,
> +                       const char *default_suffix, int64_t unit,
>                         uint64_t *result)
>   {
>       int retval;
>       const char *endptr;
> -    unsigned char c;
>       int mul_required = 0;
> +    int offset = 0;
>       long double val, mul, integral, fraction;
>   
>       retval = qemu_strtold_finite(nptr, &endptr, &val);
> @@ -226,12 +222,12 @@ static int do_strtosz(const char *nptr, const char **end,
>       if (fraction != 0) {
>           mul_required = 1;
>       }
> -    c = *endptr;
> -    mul = suffix_mul(c, unit);
> +
> +    mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit);
>       if (mul >= 0) {
> -        endptr++;
> +        endptr += offset;
>       } else {
> -        mul = suffix_mul(default_suffix, unit);
> +        mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit);
>           assert(mul >= 0);
>       }
>       if (mul == 1 && mul_required) {
> @@ -256,19 +252,35 @@ out:
>       return retval;
>   }
>   
> +/*
> + * Convert string to bytes, allowing either B/b for bytes, K/k for KB,

Then also fix here "B/b for bytes".

> + * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned

Shouldn't we refuse m/g/t? (m is the 'milli' suffix)

Thanks,

Phil.

> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
> + * other error.
> + */
> +static int do_strtosz(const char *nptr, const char **end,
> +                      const char *default_suffix, int64_t unit,
> +                      uint64_t *result)
> +{
> +    static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" };
> +
> +    return do_strtomul(nptr, end, suffixes, ARRAY_SIZE(suffixes),
> +                       default_suffix, unit, result);
> +}
> +
>   int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
>   {
> -    return do_strtosz(nptr, end, 'B', 1024, result);
> +    return do_strtosz(nptr, end, "B", 1024, result);
>   }
>   
>   int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
>   {
> -    return do_strtosz(nptr, end, 'M', 1024, result);
> +    return do_strtosz(nptr, end, "M", 1024, result);
>   }
>   
>   int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>   {
> -    return do_strtosz(nptr, end, 'B', 1000, result);
> +    return do_strtosz(nptr, end, "B", 1000, result);
>   }
>   
>   /**