[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]