Re: [PATCHv2 2/2] Simplify linuxNodeGetCPUStats

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 22.01.2014 10:53, Ján Tomko wrote:
> Split out the repetitive code.
> ---
>  src/nodeinfo.c | 77 +++++++++++++++++++++++-----------------------------------
>  1 file changed, 30 insertions(+), 47 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 9c236cd..585da49 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -683,6 +683,20 @@ cleanup:
>      return ret;
>  }
>  
> +static int
> +virNodeCPUStatsAssign(virNodeCPUStatsPtr param,
> +                      const char *name,
> +                      unsigned long long value)
> +{
> +    if (virStrcpyStatic(param->field, name) == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("Field kernel cpu time too long for destination"));

When touching this, this error message is a bit hard to read; I've to
read it multiple time to get the meaning. I suggest changing that to:

kernel cpu time field is too long for the destination



> +        return -1;
> +    }
> +    param->value = value;
> +    return 0;
> +}
> +
>  # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
>  
>  int
> @@ -721,8 +735,6 @@ linuxNodeGetCPUStats(FILE *procstat,
>          char *buf = line;
>  
>          if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */
> -            size_t i;
> -
>              if (sscanf(buf,
>                         "%*s %llu %llu %llu %llu %llu" // user ~ iowait
>                         "%llu %llu %llu %llu %llu",    // irq  ~ guest_nice
> @@ -731,51 +743,22 @@ linuxNodeGetCPUStats(FILE *procstat,
>                  continue;
>              }
>  
> -            for (i = 0; i < *nparams; i++) {
> -                virNodeCPUStatsPtr param = &params[i];
> -
> -                switch (i) {
> -                case 0: /* fill kernel cpu time here */
> -                    if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_KERNEL) == NULL) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       "%s", _("Field kernel cpu time too long for destination"));
> -                        goto cleanup;
> -                    }
> -                    param->value = (sys + irq + softirq) * TICK_TO_NSEC;
> -                    break;
> -
> -                case 1: /* fill user cpu time here */
> -                    if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_USER) == NULL) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       "%s", _("Field kernel cpu time too long for destination"));
> -                        goto cleanup;
> -                    }
> -                    param->value = (usr + ni) * TICK_TO_NSEC;
> -                    break;
> -
> -                case 2: /* fill idle cpu time here */
> -                    if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IDLE) == NULL) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       "%s", _("Field kernel cpu time too long for destination"));
> -                        goto cleanup;
> -                    }
> -                    param->value = idle * TICK_TO_NSEC;
> -                    break;
> -
> -                case 3: /* fill iowait cpu time here */
> -                    if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IOWAIT) == NULL) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       "%s", _("Field kernel cpu time too long for destination"));
> -                        goto cleanup;
> -                    }
> -                    param->value = iowait * TICK_TO_NSEC;
> -                    break;
> -
> -                default:
> -                    break;
> -                    /* should not hit here */
> -                }
> -            }
> +            if (virNodeCPUStatsAssign(&params[0], VIR_NODE_CPU_STATS_KERNEL,
> +                                      (sys + irq + softirq) * TICK_TO_NSEC) < 0)
> +                goto cleanup;
> +
> +            if (virNodeCPUStatsAssign(&params[1], VIR_NODE_CPU_STATS_USER,
> +                                      (usr + ni) * TICK_TO_NSEC) < 0)
> +                goto cleanup;
> +
> +            if (virNodeCPUStatsAssign(&params[2], VIR_NODE_CPU_STATS_IDLE,
> +                                      idle * TICK_TO_NSEC) < 0)
> +                goto cleanup;
> +
> +            if (virNodeCPUStatsAssign(&params[3], VIR_NODE_CPU_STATS_IOWAIT,
> +                                      iowait * TICK_TO_NSEC) < 0)
> +                goto cleanup;
> +
>              ret = 0;
>              goto cleanup;
>          }
> 

ACK

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list