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

[Peter Xu]

On Wed, May 10, 2017 at 01:34:18PM +0200, Markus Armbruster wrote:
> 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().
>  */

Yes this looks much better. :)

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

For above two points, actually I would prefer we don't have assertion
in this function. The reason is when this helper is used in dynamic
environment (e.g., converting user input into size strings), it can
crash the VM when assertion is triggered. To avoid that, I would like
val to be any value (very big, or negative). For very big one, I'd
like to take your suggestion to have a limit on maximum unit; for
negative values, let's just do nothing, since frexp() supports that,
and finally we'll just see something like "-20 MiB" and imho that's
better than a crash.

> 
> 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.

I didn't really find a good way to solve this, considering that it is
related to how "%g" is implemented in g_strdup_printf() (or say, it
decided to dump "1e+03" rather than "999.9" for some reason... and I
found that this condition goes away if we specify something <999.5).
So let me just keep it as it is.

This also helps to make sure we won't dump different strings after
this patch when used by print_type_size().

> 
> Preferably with a function comment:

Will add.

> 
> Reviewed-by: Markus Armbruster <address@hidden>

Thanks!

-- 
Peter Xu