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

On 5/23/19 11:34 AM, Wang Huaqiang wrote: > Refactor 'virResctrlMonitorStats' to track multiple statistical > records. > > Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; > --- >   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@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