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

[Markus Armbruster <armbru@redhat.com>]

Tao Xu <tao3.xu@intel.com> writes:

> Add do_strtomul() to convert string according to different suffixes.
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
>
> No changes in v17.
>
> 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)

This function could really use a contract comment now.

>  {
> -    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++) {

Aha: @num_suffix is the number of elements in suffixes[].

I'd put a terminating NULL into suffixes[] and dispense with
@num_suffix:

       for (i = 0; suffix[i]; i++) {

> +        suffix_len = strlen(suffixes[i]);

@suffix_len should be size_t, because that's what strlen() returns.

> +        if (g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {

@endptr is a terrible name: it points to the *beginning* of the suffix.

> +            *offset = suffix_len;

@offset is a terrible name: it provides no clue whatsoever on its
meaning.  It's actually for receiving the length of the suffix.

Please replace it by char **end, because that's how the related
functions work.

> +            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)

The function comment is barely adequate before your patch: it doesn't
explain the arguments.  Your patch adds more arguments, thus more
guesswork for the reader.

Again, I'd put a terminating NULL into suffixes[] and dispense with
@num_suffix.

>  {
>      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);

@endptr points behind the number, i.e. to the suffix, and @offset is the
length of the suffix.  Suggest to rename @endptr to @suffix, and replace
@offset by @endptr.

>      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);

Please assert "no trailing crap in @default_suffix".

>      }
>      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,
> + * 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.
> + */
> +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);
>  }
>  
>  /**