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

[Osier Yang]

于 2011年09月07日 20:37, Peter Krempa 写道:
> On 09/07/2011 01:45 PM, Osier Yang wrote:
>> 于 2011年09月06日 21:20, Peter Krempa 写道:
>>> diff --git a/tools/virsh.c b/tools/virsh.c
>>> index 629233f..458252b 100644
>>> --- a/tools/virsh.c
>>> +++ b/tools/virsh.c
>>> @@ -1054,16 +1054,46 @@ cleanup:
>>>    */
>>>   static const vshCmdInfo info_domblkstat[] = {
>>>       {"help", N_("get device block stats for a domain")},
>>> -    {"desc", N_("Get device block stats for a running domain.")},
>>> +    {"desc", N_("Get device block stats for a running domain.\n\n"
>>> +                "    Explanation of fields:\n"
>>> +                "      rd_req            - count of read operations 
>>> (old format)\n"
>>> +                "      rd_operations     - count of read operations 
>>> (new format)\n"
>>> +                "      rd_bytes          - count of read bytes\n"
>>> +                "      rd_total_times    - total time read 
>>> operations took\n"
>>
>> Should also document the units IMHO, it's nano-seconds.
> I agree, that documenting also the unit is important. I'm afraid that 
> the unit is hypervisor specific
> (by now, the code is implemented only for QEmu) and we would create a 
> precedent so that other
> hypervisors using other values will have to convert it before. I think 
> we should chose this unit wisely.
>
> On the other hand, I think that nano-seconds are okay.
>>
>>> +                "      wr_req            - count of write 
>>> operations (old format)\n"
>>> +                "      wr_operations     - count of write 
>>> operations (new format)\n"
>>> +                "      wr_bytes          - count of written bytes\n"
>>
>> Missed docs for wr_total_times.
> Will add it in v4
>>
>>> +                "      flush_operations  - count of flush 
>>> operations\n"
>>> +                "      flush_total_times - total time flush 
>>> operations took\n"
>>
>> likewise
>>
>>> +                "      errs              - error count")},
>>>       {NULL,NULL}
>>>   };
>>
>> Think it would be better if converting "rd_operations", "wr_operations",
>> and "flush_operations" to the old names. They have the same meaning,
>> and we used "rd_req", "wr_req" in virsh for a long time. After 
>> conversion,
>> we can have less and not duplicate documention.
> I am aware, that the meanings of the two fields are the same. I was 
> surprised
> to see new naming of this fields. If the new API has to stick with 
> these new names,
> I'll have to convert them, but I don't really like approach.

There is no field of the old API, virsh just printed same words of the 
struct
members. I intended to keep same with the QEMU returned keys' name.

>>
>>>   static const vshCmdOptDef opts_domblkstat[] = {
>>>       {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or 
>>> uuid")},
>>>       {"device", VSH_OT_DATA, VSH_OFLAG_REQ, N_("block device")},
>>> +    {"human",  VSH_OT_BOOL, 0, N_("print a more human readable 
>>> output")},
>>>       {NULL, 0, 0, NULL}
>>>   };
>>>
>>> +struct _domblkstat_human_readable {
>>> +    const char *field;
>>> +    const char *human;
>>> +};
>>> +
>>> +static const struct _domblkstat_human_readable 
>>> domblkstat_translate[] = {
>>> +    { VIR_DOMAIN_BLOCK_STATS_READ_BYTES,        N_("number of read 
>>> bytes:      ") }, /* 0 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_READ_REQ,          N_("number of read 
>>> operations: ") }, /* 1 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES,  N_("total duration 
>>> of reads:   ") }, /* 2 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES,       N_("number of bytes 
>>> written:   ") }, /* 3 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_WRITE_REQ,         N_("number of write 
>>> operations:") }, /* 4 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES, N_("total duration 
>>> of writes:  ") }, /* 5 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ,         N_("number of flush 
>>> operations:") }, /* 6 */
>>> +    { VIR_DOMAIN_BLOCK_STATS_FLUSH_TOTAL_TIMES, N_("total duration 
>>> of flushes: ") }, /* 7 */
>>
>> Not sure if we should keep the same docs for these fields defined in 
>> struct
>> info_domblkstat and virsh.pod? it looks a bit long though. :-)
> Yes this information is tripple-redundant :/ but I don't really see 
> other options. Maybe just drop it from
> help command as there is the --human option, or drop the human option.
>>
>>> +    { VIR_DOMAIN_BLOCK_STATS_ERRS,              N_("error 
>>> count:               ") }, /* 8 */
>>> +    { NULL, NULL }
>>> +};
>
>>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>>> index d826997..c8d88c9 100644
>>> --- a/tools/virsh.pod
>>> +++ b/tools/virsh.pod
>>> @@ -501,13 +501,27 @@ be lost once the guest stops running, but the 
>>> snapshot contents still
>>>   exist, and a new domain with the same name and UUID can restore the
>>>   snapshot metadata with B<snapshot-create>.
>>>
>>> -=item B<domblkstat>  I<domain>  I<block-device>
>>> +=item B<domblkstat>  I<domain>  I<block-device>  [I<--human>]
>>>
>>>   Get device block stats for a running domain.  A I<block-device>  
>>> corresponds
>>>   to a unique target name (<target dev='name'/>) or source file 
>>> (<source
>>>   file='name'/>) for one of the disk devices attached to I<domain>  
>>> (see
>>>   also B<domblklist>  for listing these names).
>>>
>>> +Use I<--human>  for a more human readable output.
>>> +
>>> +B<Explanation of fields:>
>>> +  rd_req            - count of read operations (old format)
>>> +  rd_operations     - count of read operations (new format)
>>> +  rd_bytes          - count of read bytes
>>> +  rd_total_times    - total time read operations took
>>
>> Units
> Fix in v4.
>>
>>> +  wr_req            - count of write operations (old format)
>>> +  wr_operations     - count of write operations (new format)
>>> +  wr_bytes          - count of written bytes
>>
>> Missed wr_total_times
> Fix in v4.
>>
>>> +  flush_operations  - count of flush operations
>>> +  flush_total_times - total time flush operations took
>>
>> Units.
> Fix in v4.
>>
>>> +  errs              - error count
>>> +
>>>   =item B<domifstat>  I<domain>  I<interface-device>
>>>
>>>   Get network interface stats for a running domain.
>>
>> Others look good to me.
>>
>> Regards
>> Osier
> Thanks for finding the small glitches.
>
> Peter