[libvirt] [PATCH v4] virsh: Add more human-friendly output of domblkstat command

[Eric Blake]

On 09/08/2011 03:11 PM, Peter Krempa wrote:
> Users of virsh complain that output of the domblkstat command
> is not intuitive enough. This patch adds explanation of fields
> returned by this command to the help section for domblkstat and
> the man page of virsh. Also a switch --human is added for
> domblkstat that prints the fields with more descriptive
> texts.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=731656
>
> Changes to v3:
> - Add units to duration values
> - Add missing write doc/stranslation
> - Add translation from new api names to legacy names used in previous
>    versions in virsh

Nice, but still not quite ready.

> +    {"desc", N_("Get device block stats for a running domain.\n\n"
> +                "    Explanation of fields:\n"
> +                "      rd_req            - count of read operations\n"

That's a bit long for 'virsh help' when compared to other commands.  I'm 
not sure if it is sufficient to just list this in virsh.pod, but I'm 
also not opposed to keeping this part of the patch as-is.

> +/* translations into a more human readable form */
> +static const struct _domblkstat_translate domblkstat_human[] = {
> +    { VIR_DOMAIN_BLOCK_STATS_READ_BYTES,        N_("number of read bytes:      ") }, /* 0 */

You are correct that you have to use N_() here to initialize the array. 
  But...

> +            if (human) {
> +                /* human friendly output */
> +                vshPrint(ctl, N_("Device: %s\n"), device);
> +
> +                if (stats.rd_req>= 0)
> +                    vshPrint (ctl, "%s %lld\n", domblkstat_human[1].to, stats.rd_req);

...that means down here, you have to translate things.  Also, your 
formatting is not typical (no space between function name and opening '('):

vshPrint(ctl, "%s %lld\n", _(domblkstat_human[1].to), stats.rd_req);

> @@ -1129,32 +1190,63 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
>           }
>
>           int i;
> +
> +        /* set for prettier output */
> +        if (human) {
> +            vshPrint(ctl, N_("Device: %s\n"), device);
> +            device = "";
> +        }
> +
>           /* XXX: The output sequence will be different. */

Can we fix this?  It would be nice if 0.9.4 and 0.9.5 output the initial 
fields in the same order when only the initial fields are available, and 
that only the presence of new fields causes the new ordering.

>           for (i = 0; i<  nparams; i++) {
> +            /* translate messages into a human readable form, if requested */
> +            field_name = NULL;
> +
> +            if (human)
> +                for (j = 0; domblkstat_human[j].from != NULL; j++)
> +                    if (STREQ(params[i].field, domblkstat_human[j].from)) {
> +                        field_name = domblkstat_human[j].to;

Again, needs translation here, since the array could not be translated:

field_name = _(domblkstat_human[j].to);

> +
> +B<Explanation of fields:>
> +  rd_req            - count of read operations
> +  rd_bytes          - count of read bytes
> +  rd_total_times    - total time read operations took (ns)
> +  wr_req            - count of write operations
> +  wr_bytes          - count of written bytes
> +  wr_total_times    - total time write operations took (ns)
> +  flush_operations  - count of flush operations
> +  flush_total_times - total time flush operations took (ns)
> +  errs              - error count

Maybe also mention that only the available fields are listed; in the 
case of older server or a hypervisor with less support, then unknown 
fields are omitted.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org