[libvirt] [PATCH v2 2/2] virsh: report only filled values in 'nodecpustats'

[Ján Tomko]

On 01/19/2014 06:55 PM, Roman Bogorodskiy wrote:
> A set of fields for CPU stats could vary on different platforms,
> for example, FreeBSD doesn't report 'iowait'.
> 
> Make virsh print out only the fields that were actually filled.

> ---
>  tools/virsh-host.c | 118 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 71 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index ac41177..8c1218e 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -34,6 +34,7 @@
>  #include "internal.h"
>  #include "virbuffer.h"
>  #include "viralloc.h"
> +#include "virhash.h"
>  #include "virsh-domain.h"
>  #include "virxml.h"
>  #include "virtypedparam.h"
> @@ -335,23 +336,15 @@ static const vshCmdOptDef opts_node_cpustats[] = {
>  static bool
>  cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd)
>  {
> -    size_t i, j;
> +    size_t i, j, k;
>      bool flag_utilization = false;
>      bool flag_percent = vshCommandOptBool(cmd, "percent");
>      int cpuNum = VIR_NODE_CPU_STATS_ALL_CPUS;
>      virNodeCPUStatsPtr params;
>      int nparams = 0;
>      bool ret = false;
> -    struct cpu_stats {
> -        unsigned long long user;
> -        unsigned long long sys;
> -        unsigned long long idle;
> -        unsigned long long iowait;
> -        unsigned long long intr;
> -        unsigned long long util;
> -    } cpu_stats[2];
> -    double user_time, sys_time, idle_time, iowait_time, intr_time, total_time;
> -    double usage;
> +    const char *fields[] = {"user:", "system:", "idle:", "iowait:", "intr:", NULL};

Maybe creating an enum with VIR_ENUM_DECL/VIR_ENUM_IMPL would make the code
easier to read? (You could use the FromString/ToString functions and an array
of bools to tell missing/zero fields apart instead of NULL pointers)

> +    virHashTablePtr cpu_stats[2];

This should be initialized to NULLs.

>  
>      if (vshCommandOptInt(cmd, "cpu", &cpuNum) < 0) {
>          vshError(ctl, "%s", _("Invalid value of cpuNum"));
> @@ -372,6 +365,10 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd)
>      params = vshCalloc(ctl, nparams, sizeof(*params));
>  
>      for (i = 0; i < 2; i++) {
> +        cpu_stats[i] = virHashCreate(0, (virHashDataFree) free);
> +        if (cpu_stats[i] == NULL)
> +            goto cleanup;
> +
>          if (i > 0)
>              sleep(1);
>  
> @@ -381,20 +378,25 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd)
>          }
>  
>          for (j = 0; j < nparams; j++) {
> -            unsigned long long value = params[j].value;
> +            unsigned long long *value;
> +
> +            if (VIR_ALLOC(value) < 0)
> +                goto cleanup;
> +
> +            *value = params[j].value;
>  
>              if (STREQ(params[j].field, VIR_NODE_CPU_STATS_KERNEL)) {
> -                cpu_stats[i].sys = value;
> +                virHashAddEntry(cpu_stats[i], "system:", value);
>              } else if (STREQ(params[j].field, VIR_NODE_CPU_STATS_USER)) {
> -                cpu_stats[i].user = value;
> +                virHashAddEntry(cpu_stats[i], "user:", value);
>              } else if (STREQ(params[j].field, VIR_NODE_CPU_STATS_IDLE)) {
> -                cpu_stats[i].idle = value;
> +                virHashAddEntry(cpu_stats[i], "idle:", value);
>              } else if (STREQ(params[j].field, VIR_NODE_CPU_STATS_IOWAIT)) {
> -                cpu_stats[i].iowait = value;
> +                virHashAddEntry(cpu_stats[i], "iowait:", value);
>              } else if (STREQ(params[j].field, VIR_NODE_CPU_STATS_INTR)) {
> -                cpu_stats[i].intr = value;
> +                virHashAddEntry(cpu_stats[i], "intr:", value);
>              } else if (STREQ(params[j].field, VIR_NODE_CPU_STATS_UTILIZATION)) {

Hmm, did libvirtd ever fill out the 'usage' field? I can't find it in git history.

> -                cpu_stats[i].util = value;
> +                virHashAddEntry(cpu_stats[i], "usage:", value);
>                  flag_utilization = true;
>              }
>          }
> @@ -405,46 +407,68 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd)


> +            virHashTablePtr diff;
This should be declared at the start of the function, so you can free it in
the cleanup label.

> +            double total_time = 0;
> +            double usage = -1;
> +
> +            diff = virHashCreate(0, (virHashDataFree) free);
> +            if (diff == NULL)
> +                goto cleanup;
> +
> +            for (k = 0; fields[k] != NULL; k++) {
> +                double *value_diff;
> +                unsigned long long *oldval = virHashLookup(cpu_stats[0], fields[k]);
> +                unsigned long long *newval = virHashLookup(cpu_stats[1], fields[k]);
> +


Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140128/68437a22/attachment-0001.sig>