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

Re: [libvirt] [PATCH 05/12] [v7] virNodeGetCPUStats: Implement virsh support



On 06/14/2011 03:06 AM, Daniel P. Berrange wrote:
> On Tue, Jun 07, 2011 at 10:02:13AM +0900, Minoru Usui wrote:
>> virNodeGetCPUStats: Implement virsh support
>>
>> Signed-off-by: Minoru Usui <usui mxm nes nec co jp>
>> ---
>>  tools/virsh.c   |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tools/virsh.pod |    7 +++
>>  2 files changed, 141 insertions(+), 0 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 5679a2d..2fbc91a 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -3682,6 +3682,139 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>>  }
>>  
>>  /*
>> + * "nodecpustats" command
>> + */
>> +static const vshCmdInfo info_nodecpustats[] = {
>> +    {"help", N_("Prints cpu stats of the node.")},
>> +    {"desc", N_("Returns cpu stats of the node.(nsec)")},

Not sure that the (nsec) string aids understanding here, so I nuked it.

>> +    struct cpu_stats {
>> +        unsigned long long user;
>> +        unsigned long long sys;
>> +        unsigned long long idle;
>> +        unsigned long long iowait;
>> +        unsigned long long util;
>> +    } cpu_stats[2];

>> +
>> +    memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2);

2 is a magic number here, easily avoided.

>> +
>> +            if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0)
>> +                cpu_stats[i].sys = value;

'make syntax-check' called you on this; use STREQ instead.

>> +
>> +            if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0)
>> +                cpu_stats[i].user = value;

For a (minor) efficiency boost, we can use 'else if' instead of 'if'.

>> +
>> +        if (flag_utilization || (flag_percent == false))
>> +            break;
>> +
>> +        i++;
>> +        sleep(1);
>> +    } while(i < 2);
>> +
>> +    if (flag_percent == false) {

Comparison against bool is discouraged in HACKING.

>> @@ -11293,6 +11426,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = {
>>      {"freecell", cmdFreecell, opts_freecell, info_freecell, 0},
>>      {"hostname", cmdHostname, NULL, info_hostname, 0},
>>      {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0},
>> +    {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats},

Sort these lines, and for consistency, keep the trailing 0.

>>      {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command,
>>       info_qemu_monitor_command, 0},
>>      {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0},
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 6ca3002..ade4eab 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -237,6 +237,13 @@ Print the XML representation of the hypervisor sysinfo, if available.
>>  Returns basic information about the node, like number and type of CPU,
>>  and size of the physical memory.
>>  
>> +=item B<nodecpustats> optional I<--cpu> I<--percent>

The usage is [--cpu] <cpu>; I've tweaked this slightly.

>> +
>> +Returns cpu stats of the node.
>> +If I<--cpu> is specified, this will prints specified cpu statistics only.
>> +If I<--percent> is specified, this will prints percentage of each kind of cpu
>> +statistics during 1 second.
>> +
>>  =item B<capabilities>
>>  
>>  Print an XML document describing the capabilities of the hypervisor
> 
> ACK

Here's what I squashed in before pushing:

diff --git i/tools/virsh.c w/tools/virsh.c
index 6071aaa..1e2385c 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -3718,7 +3718,7 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd
ATTRIBUTE_UNUSED)
  */
 static const vshCmdInfo info_nodecpustats[] = {
     {"help", N_("Prints cpu stats of the node.")},
-    {"desc", N_("Returns cpu stats of the node.(nsec)")},
+    {"desc", N_("Returns cpu stats of the node.")},
     {NULL, NULL}
 };

@@ -3766,7 +3766,7 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd
ATTRIBUTE_UNUSED)
         return true;
     }

-    memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2);
+    memset(cpu_stats, 0, sizeof(cpu_stats));
     params = vshCalloc(ctl, nparams, sizeof(*params));

     i = 0;
@@ -3779,33 +3779,29 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd
*cmd ATTRIBUTE_UNUSED)
         for (j = 0; j < nparams; j++) {
             unsigned long long value = params[j].value;

-            if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0)
+            if (STREQ(params[j].field, VIR_CPU_STATS_KERNEL)) {
                 cpu_stats[i].sys = value;
-
-            if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0)
+            } else if (STREQ(params[j].field, VIR_CPU_STATS_USER)) {
                 cpu_stats[i].user = value;
-
-            if (strcmp(params[j].field, VIR_CPU_STATS_IDLE) == 0)
+            } else if (STREQ(params[j].field, VIR_CPU_STATS_IDLE)) {
                 cpu_stats[i].idle = value;
-
-            if (strcmp(params[j].field, VIR_CPU_STATS_IOWAIT) == 0)
+            } else if (STREQ(params[j].field, VIR_CPU_STATS_IOWAIT)) {
                 cpu_stats[i].iowait = value;
-
-            if (strcmp(params[j].field, VIR_CPU_STATS_UTILIZATION) == 0) {
+            } else if (STREQ(params[j].field, VIR_CPU_STATS_UTILIZATION)) {
                 cpu_stats[i].util = value;
                 flag_utilization = true;
             }
         }

-        if (flag_utilization || (flag_percent == false))
+        if (flag_utilization || !flag_percent)
             break;

         i++;
         sleep(1);
     } while(i < 2);

-    if (flag_percent == false) {
-        if (flag_utilization == false) {
+    if (!flag_percent) {
+        if (!flag_utilization) {
             vshPrint(ctl, "%-15s %20llu\n", _("user  :"),
cpu_stats[0].user);
             vshPrint(ctl, "%-15s %20llu\n", _("system:"),
cpu_stats[0].sys);
             vshPrint(ctl, "%-15s %20llu\n", _("idle  :"),
cpu_stats[0].idle);
@@ -11526,8 +11522,8 @@ static const vshCmdDef hostAndHypervisorCmds[] = {
      VSH_CMD_FLAG_NOCONNECT},
     {"freecell", cmdFreecell, opts_freecell, info_freecell, 0},
     {"hostname", cmdHostname, NULL, info_hostname, 0},
+    {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats,
info_nodecpustats, 0},
     {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0},
-    {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats,
info_nodecpustats},
     {"qemu-monitor-command", cmdQemuMonitorCommand,
opts_qemu_monitor_command,
      info_qemu_monitor_command, 0},
     {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0},
diff --git i/tools/virsh.pod w/tools/virsh.pod
index b2dbbb9..b4c580a 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -239,10 +239,10 @@ and size of the physical memory. The output
corresponds to virNodeInfo
 structure. Specifically, the "CPU socket(s)" field means number of CPU
 sockets per NUMA cell.

-=item B<nodecpustats> optional I<--cpu> I<--percent>
+=item B<nodecpustats> optional I<cpu> I<--percent>

 Returns cpu stats of the node.
-If I<--cpu> is specified, this will prints specified cpu statistics only.
+If I<cpu> is specified, this will prints specified cpu statistics only.
 If I<--percent> is specified, this will prints percentage of each kind
of cpu
 statistics during 1 second.


-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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