[libvirt] [PATCH 7/9] util: Refactor 'virResctrlMonitorStats'

[Michal Privoznik]

On 5/23/19 11:34 AM, Wang Huaqiang wrote:
> Refactor 'virResctrlMonitorStats' to track multiple statistical
> records.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>   src/qemu/qemu_driver.c |  2 +-
>   src/util/virresctrl.c  | 17 ++++++++++++++---
>   src/util/virresctrl.h  | 12 ++++++++++--
>   3 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2abed86..4ea346c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20120,7 +20120,7 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
>                        "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j);
>               if (virTypedParamsAddUInt(&record->params, &record->nparams,
>                                         maxparams, param_name,
> -                                      resdata[i]->stats[j]->val) < 0)
> +                                      resdata[i]->stats[j]->vals[0]) < 0)

So why undergo the whole torture of 8/9 and 9/9 if we will report one 
value only?

Also, I'm not sure @vals is going to be allocated at all times, will it?

>                   goto cleanup;
>           }
>       }
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 0f18d2b..c2fe2ed 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2686,6 +2686,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>   {
>       int rv = -1;
>       int ret = -1;
> +    unsigned int val = 0;
>       DIR *dirp = NULL;
>       char *datapath = NULL;
>       char *filepath = NULL;
> @@ -2742,7 +2743,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>           if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0)
>               goto cleanup;
>   
> -        rv = virFileReadValueUint(&stat->val, "%s/%s/%s", datapath,
> +        rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
>                                     ent->d_name, resource);
>           if (rv == -2) {
>               virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2752,6 +2753,12 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>           if (rv < 0)
>               goto cleanup;
>   
> +        if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
> +            goto cleanup;
> +
> +        if (virStringListAdd(&stat->features, resource) < 0)
> +            goto cleanup;
> +
>           if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
>               goto cleanup;
>       }
> @@ -2779,8 +2786,12 @@ virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
>       if (!stats)
>           return;
>   
> -    for (i = 0; i < nstats; i++)
> -        VIR_FREE(stats[i]);
> +    for (i = 0; i < nstats; i++) {
> +        if (stats[i]) {
> +            VIR_FREE(stats[i]->vals);
> +            virStringListFree(stats[i]->features);
> +        }
> +    }
>   }
>   
>   
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index abdeb59..97205bc 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -194,8 +194,16 @@ typedef virResctrlMonitor *virResctrlMonitorPtr;
>   typedef struct _virResctrlMonitorStats virResctrlMonitorStats;
>   typedef virResctrlMonitorStats *virResctrlMonitorStatsPtr;
>   struct _virResctrlMonitorStats {
> -    unsigned int id;
> -    unsigned int val;
> +    /* The system assigned cache ID associated with statistical record */
> +     unsigned int id;
> +    /* @features is a NULL terminal string list tracking the statistical record
> +     * name.*/
> +    char **features;
> +    /* @vals store the statistical record values and @val[0] is the value for
> +     * @features[0], @val[1] for at features[1] ... respectively */
> +    unsigned int *vals;
> +    /* The length of @vals array */
> +    size_t nvals;

We like the following style more:

struct X {
   int memberA;  /* some description of this member
                    split into two lines */
   bool memberB; /* one line description */
};

But on the other hand, this is consistent with the rest of resctrl code. 
Your call which style to use.

Michal