[PATCH 0/5] Couple of virCapabilities cleanups
I'm working on reporting HMAT in virCapabilities and noticed couple of possible cleanups. I'm sending them upfront. Michal Prívozník (5): virCapabilitiesHostNUMAFormat: Swap order of arguments virCapabilitiesHostNUMAInitReal: Free @cpus properly virCapabilitiesHostNUMAAddCell: Take double pointer virCapabilitiesHostNUMAInitReal: Use g_auto* where possible virCapabilitiesHostNUMAInitReal: Bring variables into loop src/conf/capabilities.c | 75 ++++++++++++++++++---------------- src/conf/capabilities.h | 6 +-- src/libxl/libxl_capabilities.c | 4 +- src/test/test_driver.c | 5 ++- tests/testutils.c | 6 +-- 5 files changed, 50 insertions(+), 46 deletions(-) -- 2.26.3
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@redhat.com> --- 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
The @cpus variable is an array of structs in which each item contains a virBitmap member. As such it is not enough to just VIR_FREE() the array - each bitmap has to be freed too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 084e09286d..4d509a6d28 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) cleanup: virBitmapFree(cpumap); + virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); VIR_FREE(cpus); VIR_FREE(siblings); VIR_FREE(pageinfo); -- 2.26.3
On 5/5/21 4:02 AM, Michal Privoznik wrote:
The @cpus variable is an array of structs in which each item contains a virBitmap member. As such it is not enough to just VIR_FREE() the array - each bitmap has to be freed too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 084e09286d..4d509a6d28 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
cleanup: virBitmapFree(cpumap); + virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
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
VIR_FREE(cpus); VIR_FREE(siblings); VIR_FREE(pageinfo);
On 5/11/21 12:37 PM, John Ferlan wrote:
On 5/5/21 4:02 AM, Michal Privoznik wrote:
The @cpus variable is an array of structs in which each item contains a virBitmap member. As such it is not enough to just VIR_FREE() the array - each bitmap has to be freed too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 084e09286d..4d509a6d28 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) cleanup: virBitmapFree(cpumap); + virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
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.
Unless I'm missing something that's just theoretical issue. Because virCapabilitiesClearHostNUMACellCPUTopology() does check for cpus == NULL and only if it is not NULL then it looks at ncpus. And I don't see how cpus could be !NULL and ncpus < 0 at the same time. But let me see if there's something I can do. Michal
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@redhat.com> --- 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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index ea07afc920..c487229ae8 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1598,10 +1598,10 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) int n; unsigned long long memory; virCapsHostNUMACellCPU *cpus = NULL; - virBitmap *cpumap = NULL; - virCapsHostNUMACellSiblingInfo *siblings = NULL; + g_autoptr(virBitmap) cpumap = NULL; + g_autofree virCapsHostNUMACellSiblingInfo *siblings = NULL; int nsiblings = 0; - virCapsHostNUMACellPageInfo *pageinfo = NULL; + g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; int npageinfo; int ret = -1; int ncpus = 0; @@ -1652,11 +1652,8 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) ret = 0; cleanup: - virBitmapFree(cpumap); virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); VIR_FREE(cpus); - VIR_FREE(siblings); - VIR_FREE(pageinfo); return ret; } -- 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@redhat.com> --- 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
On a Wednesday in 2021, Michal Privoznik wrote:
I'm working on reporting HMAT in virCapabilities and noticed couple of possible cleanups. I'm sending them upfront.
Michal Prívozník (5): virCapabilitiesHostNUMAFormat: Swap order of arguments virCapabilitiesHostNUMAInitReal: Free @cpus properly virCapabilitiesHostNUMAAddCell: Take double pointer virCapabilitiesHostNUMAInitReal: Use g_auto* where possible virCapabilitiesHostNUMAInitReal: Bring variables into loop
src/conf/capabilities.c | 75 ++++++++++++++++++---------------- src/conf/capabilities.h | 6 +-- src/libxl/libxl_capabilities.c | 4 +- src/test/test_driver.c | 5 ++- tests/testutils.c | 6 +-- 5 files changed, 50 insertions(+), 46 deletions(-)
Beautiful. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
John Ferlan -
Ján Tomko -
Michal Privoznik -
Michal Prívozník