The rest of virCapabilities format functions take virBuffer as the first argument and struct to format as the second. Also, they accept NULL (as the second argument). Fix virCapabilitiesHostNUMAFormat() so that it follows this logic. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/capabilities.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 9bd59b3cbf..084e09286d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -794,13 +794,16 @@ virCapabilitiesAddStoragePool(virCaps *caps, static int -virCapabilitiesHostNUMAFormat(virCapsHostNUMA *caps, - virBuffer *buf) +virCapabilitiesHostNUMAFormat(virBuffer *buf, + virCapsHostNUMA *caps) { size_t i; size_t j; char *siblings; + if (!caps) + return 0; + virBufferAddLit(buf, "<topology>\n"); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<cells num='%d'>\n", caps->cells->len); @@ -1125,8 +1128,7 @@ virCapabilitiesFormatHostXML(virCapsHost *host, virBufferAsprintf(buf, "<netprefix>%s</netprefix>\n", host->netprefix); - if (host->numa && - virCapabilitiesHostNUMAFormat(host->numa, buf) < 0) + if (virCapabilitiesHostNUMAFormat(buf, host->numa) < 0) return -1; if (virCapabilitiesFormatCaches(buf, &host->cache) < 0) -- 2.26.3

On 5/5/21 4:02 AM, Michal Privoznik wrote: There's some coding axiom about for every bug you fix you introduce another ;-)... Anyway Coverity notes you can get to cleanup from within the for loop when ncpus < 0 and that will not be very good for this call. Yes, -1 can no longer be returned, but -2 can be and we could fall to cleanup (at least theoretically). Another tweak could be to only check -2 and continue in the if statement since -1 is no longer possible. John

What this function really does it takes ownership of all pointers passed (well, except for the first one - caps - to which it registers new NUMA node). But since all info is passed as a single pointer it's hard to tell (and use g_auto*). Let's use double pointers to make the ownership transfer obvious. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/capabilities.c | 45 +++++++++++++++++++--------------- src/conf/capabilities.h | 6 ++--- src/libxl/libxl_capabilities.c | 4 +-- src/test/test_driver.c | 5 ++-- tests/testutils.c | 6 ++--- 5 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 4d509a6d28..ea07afc920 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -330,36 +330,45 @@ virCapabilitiesSetNetPrefix(virCaps *caps, * @num: ID number of NUMA cell * @mem: Total size of memory in the NUMA node (in KiB) * @ncpus: number of CPUs in cell - * @cpus: array of CPU definition structures, the pointer is stolen + * @cpus: array of CPU definition structures * @nsiblings: number of sibling NUMA nodes * @siblings: info on sibling NUMA nodes * @npageinfo: number of pages at node @num * @pageinfo: info on each single memory page * - * Registers a new NUMA cell for a host, passing in a - * array of CPU IDs belonging to the cell + * Registers a new NUMA cell for a host, passing in a array of + * CPU IDs belonging to the cell, distances to other NUMA nodes + * and info on hugepages on the node. + * + * All pointers are stolen. */ void virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int num, unsigned long long mem, int ncpus, - virCapsHostNUMACellCPU *cpus, + virCapsHostNUMACellCPU **cpus, int nsiblings, - virCapsHostNUMACellSiblingInfo *siblings, + virCapsHostNUMACellSiblingInfo **siblings, int npageinfo, - virCapsHostNUMACellPageInfo *pageinfo) + virCapsHostNUMACellPageInfo **pageinfo) { virCapsHostNUMACell *cell = g_new0(virCapsHostNUMACell, 1); cell->num = num; cell->mem = mem; - cell->ncpus = ncpus; - cell->cpus = cpus; - cell->nsiblings = nsiblings; - cell->siblings = siblings; - cell->npageinfo = npageinfo; - cell->pageinfo = pageinfo; + if (cpus) { + cell->ncpus = ncpus; + cell->cpus = g_steal_pointer(cpus); + } + if (siblings) { + cell->nsiblings = nsiblings; + cell->siblings = g_steal_pointer(siblings); + } + if (pageinfo) { + cell->npageinfo = npageinfo; + cell->pageinfo = g_steal_pointer(pageinfo); + } g_ptr_array_add(caps->cells, cell); } @@ -1568,7 +1577,7 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) virCapabilitiesHostNUMAAddCell(caps, 0, nodeinfo.memory, - cid, cpus, + cid, &cpus, 0, NULL, 0, NULL); } @@ -1633,13 +1642,9 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) memory >>= 10; virCapabilitiesHostNUMAAddCell(caps, n, memory, - ncpus, cpus, - nsiblings, siblings, - npageinfo, pageinfo); - - cpus = NULL; - siblings = NULL; - pageinfo = NULL; + ncpus, &cpus, + nsiblings, &siblings, + npageinfo, &pageinfo); virBitmapFree(cpumap); cpumap = NULL; } diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 66cae4b902..ba863447c0 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -254,11 +254,11 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int num, unsigned long long mem, int ncpus, - virCapsHostNUMACellCPU *cpus, + virCapsHostNUMACellCPU **cpus, int nsiblings, - virCapsHostNUMACellSiblingInfo *siblings, + virCapsHostNUMACellSiblingInfo **siblings, int npageinfo, - virCapsHostNUMACellPageInfo *pageinfo); + virCapsHostNUMACellPageInfo **pageinfo); virCapsGuestMachine ** virCapabilitiesAllocMachines(const char *const *names, diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 6c1199552d..ea4f370a6d 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -330,8 +330,8 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) virCapabilitiesHostNUMAAddCell(caps->host.numa, i, numa_info[i].size / 1024, - nr_cpus_node[i], cpus[i], - nr_siblings, siblings, + nr_cpus_node[i], &cpus[i], + nr_siblings, &siblings, 0, NULL); /* This is safe, as the CPU list is now stored in the NUMA cell */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 15e9018803..ea5a5005e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -329,8 +329,9 @@ testBuildCapabilities(virConnectPtr conn) virCapabilitiesHostNUMAAddCell(caps->host.numa, i, privconn->cells[i].mem, - privconn->cells[i].numCpus, - cpu_cells, 0, NULL, nPages, pages); + privconn->cells[i].numCpus, &cpu_cells, + 0, NULL, + nPages, &pages); } for (i = 0; i < G_N_ELEMENTS(guest_types); i++) { diff --git a/tests/testutils.c b/tests/testutils.c index c39797e51d..d6b7c2a580 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -935,9 +935,9 @@ virTestCapsBuildNUMATopology(int seq) virCapabilitiesHostNUMAAddCell(caps, cell_id + seq, MAX_MEM_IN_CELL, - MAX_CPUS_IN_CELL, cell_cpus, - VIR_ARCH_NONE, NULL, - VIR_ARCH_NONE, NULL); + MAX_CPUS_IN_CELL, &cell_cpus, + 0, NULL, + 0, NULL); cell_cpus = NULL; } -- 2.26.3

Some variables are needed only inside for() loop. They were declared at the beginning of the function because of VIR_FREE() calls, but since they are auto-freed they can be declared inside the loop. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/capabilities.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index c487229ae8..1dae6d38cc 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1596,22 +1596,22 @@ static int virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) { int n; - unsigned long long memory; virCapsHostNUMACellCPU *cpus = NULL; - g_autoptr(virBitmap) cpumap = NULL; - g_autofree virCapsHostNUMACellSiblingInfo *siblings = NULL; - int nsiblings = 0; - g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; - int npageinfo; int ret = -1; int ncpus = 0; - int cpu; int max_node; if ((max_node = virNumaGetMaxNode()) < 0) goto cleanup; for (n = 0; n <= max_node; n++) { + g_autoptr(virBitmap) cpumap = NULL; + g_autofree virCapsHostNUMACellSiblingInfo *siblings = NULL; + int nsiblings = 0; + g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; + int npageinfo; + unsigned long long memory; + int cpu; size_t i; if ((ncpus = virNumaGetNodeCPUs(n, &cpumap)) < 0) { @@ -1645,8 +1645,6 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) ncpus, &cpus, nsiblings, &siblings, npageinfo, &pageinfo); - virBitmapFree(cpumap); - cpumap = NULL; } ret = 0; -- 2.26.3