Re: [Qemu-devel] [PATCH 07/12] qapi: add human mode to StringOutputVisitor

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]

On 01/30/2014 06:09 AM, Paolo Bonzini wrote:
> This will be used by "info qtree".  For numbers it prints both the
> decimal and hex values.  For sizes it rounds to the nearest power
> of 2^10.  For strings, it puts quotes around the string and separates
> NULL and empty string.

Nice idea!  But needs a v2.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qapi/string-output-visitor.h |  2 +-
>  include/qom/object.h                 |  3 +-
>  qapi/string-output-visitor.c         | 55 ++++++++++++++++++++++++++++++++++--
>  qdev-monitor.c                       |  2 +-
>  qom/object.c                         |  4 +--
>  tests/test-string-output-visitor.c   |  2 +-
>  tests/test-visitor-serialization.c   |  2 +-
>  7 files changed, 60 insertions(+), 10 deletions(-)
> 

> +static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
> +                           Error **errp)
> +{
> +    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
> +    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T' };

Any reason you don't include 'P' and 'E' at the end of this array?  We
parse them just fine (at least in some contexts, going by the fact that
qemu-common.h has a STRTOSZ_DEFSUFFIX_ value for them), so we also ought
to output them.

> +    uint64_t div, val;
> +    char *out;
> +    int i;
> +
> +    if (!sov->human) {
> +        out = g_strdup_printf("%llu", (long long) *obj);

(unsigned long long) would look better for the cast.

> +        string_output_set(sov, out);
> +        return;
> +    }
> +
> +    val = *obj;
> +
> +    /* Compute floor(log2(val)).  */
> +    i = 64 - clz64(val);
> +
> +    /* Find the power of 1024 that we'll display as the units.  */
> +    i /= 10;
> +    if (i >= ARRAY_SIZE(suffixes)) {
> +        i = ARRAY_SIZE(suffixes) - 1;
> +    }
> +    div = 1ULL << (i * 10);
> +
> +    out = g_strdup_printf("%0.03f%c", (double)val/div, suffixes[i]);

If val < 1024, this gives dumb output: 1.000B.  Should you special case
bytes?

Also, I like how your int printout was both decimal and hex; but here
you are throwing away information (and the bigger the number, the more
we lose as to how much got rounded away).  I'd rather see this as:

"%0.03f%c (%llu)"

so that we also have access to the unrounded exact amount.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]