Re: [Qemu-devel] [PATCH v6 2/3] utils: provide size_to_str()

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> Moving the algorithm from print_type_size() into size_to_str() so that
> other component can also leverage it. With that, refactor
> print_type_size().
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  include/qemu-common.h        |  1 +
>  qapi/string-output-visitor.c | 22 ++++++----------------
>  util/cutils.c                | 23 +++++++++++++++++++++++
>  3 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d218821..d7d0448 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -145,6 +145,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char 
> *prefix, size_t size);
>  int parse_debug_env(const char *name, int max, int initial);
>  
>  const char *qemu_ether_ntoa(const MACAddr *mac);
> +char *size_to_str(double val);
>  void page_size_init(void);
>  
>  /* returns non-zero if dump is in progress, otherwise zero is
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac821..53c2175 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -211,10 +211,8 @@ static void print_type_size(Visitor *v, const char 
> *name, uint64_t *obj,
>                              Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> -    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
> -    uint64_t div, val;
> -    char *out;
> -    int i;
> +    uint64_t val;
> +    char *out, *psize;
>  
>      if (!sov->human) {
>          out = g_strdup_printf("%"PRIu64, *obj);
> @@ -223,19 +221,11 @@ static void print_type_size(Visitor *v, const char 
> *name, uint64_t *obj,
>      }
>  
>      val = *obj;
> -
> -    /* The exponent (returned in i) minus one gives us
> -     * floor(log2(val * 1024 / 1000).  The correction makes us
> -     * switch to the higher power when the integer part is >= 1000.
> -     */
> -    frexp(val / (1000.0 / 1024.0), &i);
> -    i = (i - 1) / 10;
> -    assert(i < ARRAY_SIZE(suffixes));
> -    div = 1ULL << (i * 10);
> -
> -    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
> -                          (double)val/div, suffixes[i], i ? "iB" : "");
> +    psize = size_to_str(val);
> +    out = g_strdup_printf("%"PRIu64" (%s)", val, psize);
>      string_output_set(sov, out);
> +
> +    g_free(psize);
>  }
>  
>  static void print_type_bool(Visitor *v, const char *name, bool *obj,
> diff --git a/util/cutils.c b/util/cutils.c
> index 50ad179..c562d87 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -619,3 +619,26 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>  
>      return ret;
>  }
> +
> +/*
> + * Please free the buffer after use.
> + */

A proper function comment would be nice.  Something like

/*
 * Return human readable string for size @val.
 * @val must be between 0 inclusive and 1000EiB exclusive.
 * Use IEC binary units like KiB, MiB, and so forth
 * Caller is responsible for passing it to g_free().
 */

> +char *size_to_str(double val)
> +{
> +    static const char *suffixes[] = { "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei" 
> };
> +    unsigned long div;
> +    int i;
> +
> +    /*
> +     * The exponent (returned in i) minus one gives us
> +     * floor(log2(val * 1024 / 1000).  The correction makes us
> +     * switch to the higher power when the integer part is >= 1000.
> +     * (see e41b509d68afb1f for more info)
> +     */
> +    frexp(val / (1000.0 / 1024.0), &i);
> +    i = (i - 1) / 10;
> +    assert(i < ARRAY_SIZE(suffixes));
> +    div = 1ULL << (i * 10);
> +
> +    return g_strdup_printf("%0.3g %sB", val / div, suffixes[i]);
> +}

The function happily accepts negative sizes.  You can reject them with
assert(), adapt my function comment accordingly, or do nothing.

The function asserts on very large sizes.  I'm fine with that, and
worded my function comment accordingly.  If you want to avoid the
constraint, format them in the biggest unit, e.g. 3.1415 * (1ull<70) as
"3142 EiB", in followup patch.

The function formats 999.9 * (1ull<<60) as "1e+03 EiB".  Oops.  No big
deal, but you might want to fix it anyway, possibly in a followup patch.

Preferably with a function comment:

Reviewed-by: Markus Armbruster <address@hidden>