[libvirt] [PATCH v3] virsh: Add more human-friendly output of domblkstat command
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 v2: - Modify for new fields in virDomainBlockStatsFlags Changes to v1: - Rebase to current head --- tools/virsh.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 16 +++++++- 2 files changed, 112 insertions(+), 18 deletions(-) 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" + " 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" + " flush_operations - count of flush operations\n" + " flush_total_times - total time flush operations took\n" + " errs - error count")}, {NULL,NULL} }; 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 */ + { VIR_DOMAIN_BLOCK_STATS_ERRS, N_("error count: ") }, /* 8 */ + { NULL, NULL } +}; + static bool cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) { @@ -1071,8 +1101,11 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) const char *name = NULL, *device = NULL; struct _virDomainBlockStats stats; virTypedParameterPtr params = NULL; + const char *field_name = NULL; + int j = 0; int rc, nparams = 0; bool ret = false; + bool human = vshCommandOptBool(cmd, "human"); /* enable human readable output */ if (!vshConnectionUsability (ctl, ctl->conn)) return false; @@ -1104,20 +1137,41 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (stats.rd_req >= 0) - vshPrint (ctl, "%s rd_req %lld\n", device, stats.rd_req); + if (human) { + /* human friendly output */ + vshPrint(ctl, N_("Device: %s\n"), device); + + if (stats.rd_req >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[1].human, stats.rd_req); + + if (stats.rd_bytes >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[0].human, stats.rd_bytes); + + if (stats.wr_req >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[4].human, stats.wr_req); + + if (stats.wr_bytes >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[3].human, stats.wr_bytes); + + if (stats.errs >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[8].human, stats.errs); + } else { + + if (stats.rd_req >= 0) + vshPrint (ctl, "%s rd_req %lld\n", device, stats.rd_req); - if (stats.rd_bytes >= 0) - vshPrint (ctl, "%s rd_bytes %lld\n", device, stats.rd_bytes); + if (stats.rd_bytes >= 0) + vshPrint (ctl, "%s rd_bytes %lld\n", device, stats.rd_bytes); - if (stats.wr_req >= 0) - vshPrint (ctl, "%s wr_req %lld\n", device, stats.wr_req); + if (stats.wr_req >= 0) + vshPrint (ctl, "%s wr_req %lld\n", device, stats.wr_req); - if (stats.wr_bytes >= 0) - vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes); + if (stats.wr_bytes >= 0) + vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes); - if (stats.errs >= 0) - vshPrint (ctl, "%s errs %lld\n", device, stats.errs); + if (stats.errs >= 0) + vshPrint (ctl, "%s errs %lld\n", device, stats.errs); + } } } else { params = vshMalloc(ctl, sizeof(*params) * nparams); @@ -1129,32 +1183,58 @@ 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. */ for (i = 0; i < nparams; i++) { + /* translate messages into a human readable form, if requested */ + if (human) { + /* try to look up the translation into a more human readable form */ + field_name = NULL; + for (j = 0; domblkstat_translate[j].field != NULL; j++) { + if (STREQ(params[i].field, domblkstat_translate[j].field)) { + field_name = domblkstat_translate[j].human; + + break; + } + } + + /* translation not found, stick with the default field name */ + if (!field_name) + field_name = params[i].field; + } else { + field_name = params[i].field; + } + switch(params[i].type) { case VIR_TYPED_PARAM_INT: vshPrint (ctl, "%s %s %d\n", device, - params[i].field, params[i].value.i); + field_name, params[i].value.i); break; case VIR_TYPED_PARAM_UINT: vshPrint (ctl, "%s %s %u\n", device, - params[i].field, params[i].value.ui); + field_name, params[i].value.ui); break; case VIR_TYPED_PARAM_LLONG: vshPrint (ctl, "%s %s %lld\n", device, - params[i].field, params[i].value.l); + field_name, params[i].value.l); break; case VIR_TYPED_PARAM_ULLONG: vshPrint (ctl, "%s %s %llu\n", device, - params[i].field, params[i].value.ul); + field_name, params[i].value.ul); break; case VIR_TYPED_PARAM_DOUBLE: vshPrint (ctl, "%s %s %f\n", device, - params[i].field, params[i].value.d); + field_name, params[i].value.d); break; case VIR_TYPED_PARAM_BOOLEAN: vshPrint (ctl, "%s %s %s\n", device, - params[i].field, params[i].value.b ? _("yes") : _("no")); + field_name, params[i].value.b ? _("yes") : _("no")); break; default: vshError(ctl, _("unimplemented block statistics parameter type")); 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 + wr_req - count of write operations (old format) + wr_operations - count of write operations (new format) + wr_bytes - count of written bytes + flush_operations - count of flush operations + flush_total_times - total time flush operations took + errs - error count + =item B<domifstat> I<domain> I<interface-device> Get network interface stats for a running domain. -- 1.7.3.4
于 2011年09月06日 21:20, Peter Krempa 写道:
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 v2: - Modify for new fields in virDomainBlockStatsFlags
Changes to v1: - Rebase to current head --- tools/virsh.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 16 +++++++- 2 files changed, 112 insertions(+), 18 deletions(-)
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.
+ " 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.
+ " 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.
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. :-)
+ { VIR_DOMAIN_BLOCK_STATS_ERRS, N_("error count: ") }, /* 8 */ + { NULL, NULL } +}; + static bool cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) { @@ -1071,8 +1101,11 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) const char *name = NULL, *device = NULL; struct _virDomainBlockStats stats; virTypedParameterPtr params = NULL; + const char *field_name = NULL; + int j = 0; int rc, nparams = 0; bool ret = false; + bool human = vshCommandOptBool(cmd, "human"); /* enable human readable output */
if (!vshConnectionUsability (ctl, ctl->conn)) return false; @@ -1104,20 +1137,41 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- if (stats.rd_req>= 0) - vshPrint (ctl, "%s rd_req %lld\n", device, stats.rd_req); + if (human) { + /* human friendly output */ + vshPrint(ctl, N_("Device: %s\n"), device); + + if (stats.rd_req>= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[1].human, stats.rd_req); + + if (stats.rd_bytes>= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[0].human, stats.rd_bytes); + + if (stats.wr_req>= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[4].human, stats.wr_req); + + if (stats.wr_bytes>= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[3].human, stats.wr_bytes); + + if (stats.errs>= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_translate[8].human, stats.errs); + } else { + + if (stats.rd_req>= 0) + vshPrint (ctl, "%s rd_req %lld\n", device, stats.rd_req);
- if (stats.rd_bytes>= 0) - vshPrint (ctl, "%s rd_bytes %lld\n", device, stats.rd_bytes); + if (stats.rd_bytes>= 0) + vshPrint (ctl, "%s rd_bytes %lld\n", device, stats.rd_bytes);
- if (stats.wr_req>= 0) - vshPrint (ctl, "%s wr_req %lld\n", device, stats.wr_req); + if (stats.wr_req>= 0) + vshPrint (ctl, "%s wr_req %lld\n", device, stats.wr_req);
- if (stats.wr_bytes>= 0) - vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes); + if (stats.wr_bytes>= 0) + vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes);
- if (stats.errs>= 0) - vshPrint (ctl, "%s errs %lld\n", device, stats.errs); + if (stats.errs>= 0) + vshPrint (ctl, "%s errs %lld\n", device, stats.errs); + } } } else { params = vshMalloc(ctl, sizeof(*params) * nparams); @@ -1129,32 +1183,58 @@ 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. */ for (i = 0; i< nparams; i++) { + /* translate messages into a human readable form, if requested */ + if (human) { + /* try to look up the translation into a more human readable form */ + field_name = NULL; + for (j = 0; domblkstat_translate[j].field != NULL; j++) { + if (STREQ(params[i].field, domblkstat_translate[j].field)) { + field_name = domblkstat_translate[j].human; + + break; + } + } + + /* translation not found, stick with the default field name */ + if (!field_name) + field_name = params[i].field; + } else { + field_name = params[i].field;
Probly should do the conversion here.
+ } + switch(params[i].type) { case VIR_TYPED_PARAM_INT: vshPrint (ctl, "%s %s %d\n", device, - params[i].field, params[i].value.i); + field_name, params[i].value.i); break; case VIR_TYPED_PARAM_UINT: vshPrint (ctl, "%s %s %u\n", device, - params[i].field, params[i].value.ui); + field_name, params[i].value.ui); break; case VIR_TYPED_PARAM_LLONG: vshPrint (ctl, "%s %s %lld\n", device, - params[i].field, params[i].value.l); + field_name, params[i].value.l); break; case VIR_TYPED_PARAM_ULLONG: vshPrint (ctl, "%s %s %llu\n", device, - params[i].field, params[i].value.ul); + field_name, params[i].value.ul); break; case VIR_TYPED_PARAM_DOUBLE: vshPrint (ctl, "%s %s %f\n", device, - params[i].field, params[i].value.d); + field_name, params[i].value.d); break; case VIR_TYPED_PARAM_BOOLEAN: vshPrint (ctl, "%s %s %s\n", device, - params[i].field, params[i].value.b ? _("yes") : _("no")); + field_name, params[i].value.b ? _("yes") : _("no")); break; default: vshError(ctl, _("unimplemented block statistics parameter type")); 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
+ 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
+ flush_operations - count of flush operations + flush_total_times - total time flush operations took
Units.
+ 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
于 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
On 09/07/2011 01:45 PM, Osier Yang wrote: 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
于 2011年09月07日 20:37, Peter Krempa 写道:
于 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
On 09/07/2011 01:45 PM, Osier Yang wrote: 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
participants (2)
-
Osier Yang -
Peter Krempa