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

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



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


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