[libvirt] [PATCH v4] 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 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 Changes to v2: - Modify for new fields in virDomainBlockStatsFlags Changes to v1: - Rebase to current head --- tools/virsh.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 15 ++++++- 2 files changed, 123 insertions(+), 18 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index cf3e816..f281d7a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1054,16 +1054,53 @@ 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\n" + " rd_bytes - count of read bytes\n" + " rd_total_times - total time read operations took (ns)\n" + " wr_req - count of write operations\n" + " wr_bytes - count of written bytes\n" + " wr_total_times - total time write operations took (ns)\n" + " flush_operations - count of flush operations\n" + " flush_total_times - total time flush operations took (ns)\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_translate { + const char *from; + const char *to; +}; + +/* 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 */ + { 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 } +}; + +/* translations into legacy field values used in previous versions */ +static const struct _domblkstat_translate domblkstat_legacy[] = { + { VIR_DOMAIN_BLOCK_STATS_READ_REQ, "rd_req"}, + { VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, "wr_req"}, + { NULL, NULL } +}; + static bool cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) { @@ -1071,8 +1108,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 +1144,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_human[1].to, stats.rd_req); + + if (stats.rd_bytes >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_human[0].to, stats.rd_bytes); + + if (stats.wr_req >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_human[4].to, stats.wr_req); + + if (stats.wr_bytes >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_human[3].to, stats.wr_bytes); + + if (stats.errs >= 0) + vshPrint (ctl, "%s %lld\n", domblkstat_human[8].to, 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 +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. */ 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; + + break; + } + + /* translate new QEmu style field names into legacy names used by virsh */ + if (!field_name) + for (j = 0; domblkstat_legacy[j].from != NULL; j++) + if (STREQ(params[i].field, domblkstat_legacy[j].from)) { + field_name = domblkstat_legacy[j].to; + + break; + } + + /* translation not found, stick with the default field name */ + if (!field_name) + 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 e82567d..a2f87cd 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -501,13 +501,26 @@ 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 + 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 + =item B<domifstat> I<domain> I<interface-device> Get network interface stats for a running domain. -- 1.7.3.4
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
participants (2)
-
Eric Blake -
Peter Krempa