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@xxxxxxxxx>
> > ---
> > 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
