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

[Peter Krempa]

On 09/08/2011 04:24 PM, Eric Blake wrote:
> On 09/08/2011 03:11 PM, Peter Krempa wrote:
>
>> +    {"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.
>
This part probably could be removed in the favor of the --human 
parameter and the original texts of the fields would be explained only 
in the man-page.
>> +/* 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 
> '('):
Uhm, I'll have to look into how the translation macros work, I learned 
from code, but it was insufficient.

I didn't notice there was a space after function name. It survived my 
copy&paste&fix. :/
>
>> +
>>           /* 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 the sake of backward compatibility, it'd be the best, but the output 
format of the new api would require even more string comparisions to 
ensure tie fields are in correct order.  (I suppose I can't rely on 
every hypervisor driver to feed these in same order).

This is managable, but i think it won't be very nice.

>>           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
You're right, I'll add that in the next version.

Peter