Tao Xu <tao3...@intel.com> writes:
> Add do_strtomul() to convert string according to different suffixes.
>
> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> Signed-off-by: Tao Xu <tao3...@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);
> }
>
> /**
