7:37 a.m.
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.
> 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