[PATCH v2 09/11] test_driver: Implement virConnectGetAllDomainStats

[Luke Yue]

On Wed, 2021-08-11 at 15:23 +0200, Martin Kletzander wrote:
> On Thu, Jul 29, 2021 at 08:10:56PM +0800, Luke Yue wrote:
> > Implement virConnectGetAllDomainStats in a modular way just like
> > QEMU
> > driver, though remove some params in GetStatsWorker that we don't
> > need
> > in test driver currently.
> > 
> > Only add the worker to get state so far, more worker will be added
> > in the future.
> > 
> 
> Sorry for not getting to this earlier.  I did not review the series
> yet,
> but there were some conflicts when applying and the patches did not
> compile for me, so I figured I'll let you know at least.
> 
> > Signed-off-by: Luke Yue <lukedyue at gmail.com>
> > ---
> > src/test/test_driver.c | 132
> > +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 132 insertions(+)
> > 
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index ca36e655f4..5da7f23a9b 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> 
> [...]
> 
> > +static int
> > +testConnectGetAllDomainStats(virConnectPtr conn,
> > +                             virDomainPtr *doms,
> > +                             unsigned int ndoms,
> > +                             unsigned int stats,
> > +                             virDomainStatsRecordPtr **retStats,
> > +                             unsigned int flags)
> > +{
> > +    testDriver *driver = conn->privateData;
> > +    unsigned int lflags = flags &
> > (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
> > +                                  
> > VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> > +                                  
> > VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
> > +
> > +    unsigned int supported = VIR_DOMAIN_STATS_STATE;
> > +    virDomainObj **vms = NULL;
> > +    size_t nvms;
> > +    virDomainStatsRecordPtr *tmpstats = NULL;
> > +    int nstats = 0;
> > +    int ret = -1;
> > +    size_t i;
> > +
> > +    virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
> > +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> > +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE |
> > +                  VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT |
> > +                  VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING |
> > +                  VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS,
> > -1);
> > +
> > +    if (!stats) {
> > +        stats = supported;
> > +    } else if ((flags &
> > VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS) &&
> > +               (stats & ~supported)) {
> > +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> > +                       _("Stats types bits 0x%x are not supported
> > by this daemon"),
> > +                       stats & ~supported);
> > +        return -1;
> > +    }
> > +
> > +    if (ndoms) {
> > +        if (virDomainObjListConvert(driver->domains, conn, doms,
> > ndoms, &vms,
> > +                                    &nvms, NULL, lflags, true) <
> > 0)
> > +            return -1;
> > +    } else {
> > +        if (virDomainObjListCollect(driver->domains, conn, &vms,
> > &nvms,
> > +                                    NULL, lflags) < 0)
> > +            return -1;
> > +    }
> > +
> > +    tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1);
> > +
> > +    for (i = 0; i < nvms; i++) {
> > +        virDomainStatsRecordPtr tmp;
> > +        virDomainObj *vm = vms[i];
> > +
> > +        virObjectLock(vm);
> > +        testDomainGetStats(conn, vm, stats, &tmp);
> 
> This function can fail and there is no way of knowing without
> checking
> the error code.  It might look like it is fine because &tmp will be
> NULL, but the function does not clear it and you did not initialise
> it
> here.  Initialisation to sensible defaults is almost always good,
> even
> if you just override it later on.
> 
> > +        virObjectUnlock(vm);
> > +
> > +        if (!tmp)
> > +            goto cleanup;
> > +
> > +        tmpstats[nstats++] = tmp;
> 
> And due to above reasons I get this here:
> 
> ../src/test/test_driver.c: In function
> ‘testConnectGetAllDomainStats’:
> ../src/test/test_driver.c:9952:28: error: ‘tmp’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>   9952 |         tmpstats[nstats++] = tmp;
>        |         ~~~~~~~~~~~~~~~~~~~^~~~~
> 
> I am compiling it with GCC 11.2.0.  But it should also fail in our
> CI,
> which would make life easier for you, so I suggest you set up a
> GitLab
> account and setup a CI for your branches there.  You might go the
> extra
> mile and set up cirrus testing as described somewhere under ci/, if
> you
> want.

Sorry for that, I will fix that and remember to initialise these in the
future. I just find out that when configured with `meson build -
Dbuildtype=debug`, it won't give this error message, but with `meson
build -Dbuildtype=release` will. I do have set up my GitLab account,
though sometimes I forgot to push my local branches to remote, I will
push them before I try to submit patches, so I can find out if the CI
passed or not.

> 
> I would suggest checking the return value _and_ initialising the
> pointer
> to NULL.  And I promise I'll have a look at the series ASAP if you
> rebase it to current master ;)
> 

Thanks, I will rebase the series and apply your suggestions in v3.

> Thanks,
> Martin