On 09/19/2011 06:23 AM, 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.
This patch also changes sequence of the output fields and their names back to the order and spelling established by previous versions of virsh to mantain compatibility with scripts.
s/mantain/maintain/
Example of human readable output:
virsh # domblkstat 1 vda --human Device: vda number of read operations: 3726 number of read bytes: 82815488 number of write operations: 478 number of bytes written: 4965376
For consistency, I think "bytes read" matches better with "bytes written".
IMHO this patch is worth getting into 0.9.5 as it fixes the changed output order since addition of the new api, and translates field names into those that have been used in previous versions.
Agree. So I'll help out, so you don't have to make a v8 :)
+++ b/tools/virsh.c @@ -369,6 +369,8 @@ static const char *vshDomainStateReasonToString(int state, int reason); static const char *vshDomainControlStateToString(int state); static const char *vshDomainVcpuStateToString(int state); static bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn); +static virTypedParameterPtr vshFindTypedParamByName(const char *name, virTypedParameterPtr list, int count);
Long line; wrapped to keep at 80 columns.
+static char *vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item);
static char *editWriteToTempFile (vshControl *ctl, const char *doc); static int editFile (vshControl *ctl, const char *filename); @@ -1054,16 +1056,43 @@ 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. See man page or use --human for explanation of fields")},
Another long line.
+ +#define DOMBLKSTAT_LEGACY_PRINT(ID, VALUE) \ + if (VALUE>= 0) \ + vshPrint(ctl, "%s %s %lld\n", device, \ + human?_(domblkstat_output[ID].human):domblkstat_output[ID].legacy, \
Spacing around ?: operator.
- if (stats.wr_bytes>= 0) - vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes); + if (virDomainBlockStats (dom, device,&stats,
No space before '(' in function call.
+ /* at first print all known values in desired order */ + for (i = 0; domblkstat_output[i].field != NULL; i++) { + if (!(par = vshFindTypedParamByName(domblkstat_output[i].field, + params, + nparams)))
Indentation.
+ if (strlen(params[i].field) == 0)
More efficient as: if (!*params[i].field)
@@ -15202,6 +15245,85 @@ vshDomainStateReasonToString(int state, int reason) return N_("unknown"); }
+static char * +vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) +{ + int size = 0; + char *str = NULL; + + if (!ctl || !item) + return NULL; + + switch(item->type) { + case VIR_TYPED_PARAM_INT: + size = snprintf(NULL, 0, "%d", item->value.i); + str = vshMalloc(ctl, size+1); + snprintf(str, size+1, "%d", item->value.i);
Yuck. Why not just use virAsprintf?
+++ b/tools/virsh.pod @@ -501,13 +501,31 @@ 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. + +Availabilty of these fields depends on hypervisor. Unsupported fields are
s/Availabilty/Availability/
+missing from the output. Other fields may appear if comunicating with a newer
s/comunicating/communicating/ ACK with my nits fixed, so I've pushed this. Here's what I squashed in: diff --git i/tools/virsh.c w/tools/virsh.c index f2bb2db..df1e10e 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -369,7 +369,9 @@ static const char *vshDomainStateReasonToString(int state, int reason); static const char *vshDomainControlStateToString(int state); static const char *vshDomainVcpuStateToString(int state); static bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn); -static virTypedParameterPtr vshFindTypedParamByName(const char *name, virTypedParameterPtr list, int count); +static virTypedParameterPtr vshFindTypedParamByName(const char *name, + virTypedParameterPtr list, + int count); static char *vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item); static char *editWriteToTempFile (vshControl *ctl, const char *doc); @@ -1056,7 +1058,8 @@ 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. See man page or use --human for explanation of fields")}, + {"desc", N_("Get device block stats for a running domain. See man page or " + "use --human for explanation of fields")}, {NULL,NULL} }; @@ -1073,24 +1076,35 @@ struct _domblkstat_sequence { const char *human; /* human-friendly explanation */ }; -/* sequence of values for output to honor legacy format from previous versions */ +/* sequence of values for output to honor legacy format from previous + * versions */ static const struct _domblkstat_sequence domblkstat_output[] = { - { VIR_DOMAIN_BLOCK_STATS_READ_REQ, "rd_req", N_("number of read operations: ") }, /* 0 */ - { VIR_DOMAIN_BLOCK_STATS_READ_BYTES, "rd_bytes", N_("number of read bytes: ") }, /* 1 */ - { VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, "wr_req", N_("number of write operations: ") }, /* 2 */ - { VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, "wr_bytes", N_("number of bytes written: ") }, /* 3 */ - { VIR_DOMAIN_BLOCK_STATS_ERRS, "errs", N_("error count: ") }, /* 4 */ - { VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ, NULL, N_("number of flush operations: ") }, /* 5 */ - { VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES, NULL, N_("total duration of reads (ns): ") }, /* 6 */ - { VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES, NULL, N_("total duration of writes (ns): ") }, /* 7 */ - { VIR_DOMAIN_BLOCK_STATS_FLUSH_TOTAL_TIMES, NULL, N_("total duration of flushes (ns):") }, /* 8 */ + { VIR_DOMAIN_BLOCK_STATS_READ_REQ, "rd_req", + N_("number of read operations: ") }, /* 0 */ + { VIR_DOMAIN_BLOCK_STATS_READ_BYTES, "rd_bytes", + N_("number of read bytes: ") }, /* 1 */ + { VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, "wr_req", + N_("number of write operations: ") }, /* 2 */ + { VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, "wr_bytes", + N_("number of bytes written: ") }, /* 3 */ + { VIR_DOMAIN_BLOCK_STATS_ERRS, "errs", + N_("error count: ") }, /* 4 */ + { VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ, NULL, + N_("number of flush operations: ") }, /* 5 */ + { VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES, NULL, + N_("total duration of reads (ns): ") }, /* 6 */ + { VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES, NULL, + N_("total duration of writes (ns): ") }, /* 7 */ + { VIR_DOMAIN_BLOCK_STATS_FLUSH_TOTAL_TIMES, NULL, + N_("total duration of flushes (ns):") }, /* 8 */ { NULL, NULL, NULL } }; -#define DOMBLKSTAT_LEGACY_PRINT(ID, VALUE) \ - if (VALUE >= 0) \ - vshPrint(ctl, "%s %s %lld\n", device, \ - human?_(domblkstat_output[ID].human):domblkstat_output[ID].legacy, \ +#define DOMBLKSTAT_LEGACY_PRINT(ID, VALUE) \ + if (VALUE >= 0) \ + vshPrint(ctl, "%s %s %lld\n", device, \ + human ? _(domblkstat_output[ID].human) \ + : domblkstat_output[ID].legacy, \ VALUE); static bool @@ -1106,15 +1120,15 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) int rc, nparams = 0; int i = 0; bool ret = false; - bool human = vshCommandOptBool(cmd, "human"); /* enable human readable output */ + bool human = vshCommandOptBool(cmd, "human"); /* human readable output */ - if (!vshConnectionUsability (ctl, ctl->conn)) + if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (!(dom = vshCommandOptDomain (ctl, cmd, &name))) + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if (vshCommandOptString (cmd, "device", &device) <= 0) + if (vshCommandOptString(cmd, "device", &device) <= 0) goto cleanup; rc = virDomainBlockStatsFlags(dom, device, NULL, &nparams, 0); @@ -1131,8 +1145,8 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) virFreeError(last_error); last_error = NULL; - if (virDomainBlockStats (dom, device, &stats, - sizeof stats) == -1) { + if (virDomainBlockStats(dom, device, &stats, + sizeof stats) == -1) { vshError(ctl, _("Failed to get block stats %s %s"), name, device); goto cleanup; @@ -1166,8 +1180,8 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) /* at first print all known values in desired order */ for (i = 0; domblkstat_output[i].field != NULL; i++) { if (!(par = vshFindTypedParamByName(domblkstat_output[i].field, - params, - nparams))) + params, + nparams))) continue; if (!(value = vshGetTypedParamValue(ctl, par))) @@ -1192,9 +1206,9 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) VIR_FREE(value); } - /* go through the fields again, looking for fields that were not printed*/ + /* go through the fields again, for remaining fields */ for (i = 0; i < nparams; i++) { - if (strlen(params[i].field) == 0) + if (!*params[i].field) continue; if (!(value = vshGetTypedParamValue(ctl, params+i))) @@ -15248,7 +15262,7 @@ vshDomainStateReasonToString(int state, int reason) static char * vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) { - int size = 0; + int ret = 0; char *str = NULL; if (!ctl || !item) @@ -15256,52 +15270,36 @@ vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) switch(item->type) { case VIR_TYPED_PARAM_INT: - size = snprintf(NULL, 0, "%d", item->value.i); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%d", item->value.i); - return str; + ret = virAsprintf(&str, "%d", item->value.i); break; case VIR_TYPED_PARAM_UINT: - size = snprintf(NULL, 0, "%u", item->value.ui); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%u", item->value.ui); - return str; + ret = virAsprintf(&str, "%u", item->value.ui); break; case VIR_TYPED_PARAM_LLONG: - size = snprintf(NULL, 0, "%lld", item->value.l); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%lld", item->value.l); - return str; + ret = virAsprintf(&str, "%lld", item->value.l); break; case VIR_TYPED_PARAM_ULLONG: - size = snprintf(NULL, 0, "%llu", item->value.ul); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%llu", item->value.ul); - return str; + ret = virAsprintf(&str, "%llu", item->value.ul); break; case VIR_TYPED_PARAM_DOUBLE: - size = snprintf(NULL, 0, "%f", item->value.d); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%f", item->value.d); - return str; + ret = virAsprintf(&str, "%f", item->value.d); break; case VIR_TYPED_PARAM_BOOLEAN: - size = snprintf(NULL, 0, "%s", item->value.b ? _("yes") : _("no")); - str = vshMalloc(ctl, size+1); - snprintf(str, size+1, "%s", item->value.b ? _("yes") : _("no")); - return str; + ret = virAsprintf(&str, "%s", item->value.b ? _("yes") : _("no")); break; default: vshError(ctl, _("unimplemented block statistics parameter type")); } - return NULL; + if (ret < 0) + vshError(ctl, "%s", _("Out of memory")); + return str; } static virTypedParameterPtr diff --git i/tools/virsh.pod w/tools/virsh.pod index edf451f..6529cd6 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -510,8 +510,8 @@ also B<domblklist> for listing these names). Use I<--human> for a more human readable output. -Availabilty of these fields depends on hypervisor. Unsupported fields are -missing from the output. Other fields may appear if comunicating with a newer +Availability of these fields depends on hypervisor. Unsupported fields are +missing from the output. Other fields may appear if communicating with a newer version of libvirtd. B<Explanation of fields> (fields appear in the folowing order): -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org