[PATCH v2 09/11] test_driver: Implement virConnectGetAllDomainStats

[Martin Kletzander]

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.

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,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210811/77ae8838/attachment-0001.sig>