[PATCH] capabilities: use g_autofree in capabilities.c
Use g_autofree in capabilities.c for some pointers still using manual cleanup, and remove unnecessary cleanup. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/capabilities.c | 44 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1a7ebf4a13..e56049687b 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1489,15 +1489,14 @@ virCapabilitiesGetNUMADistances(int node, virNumaDistance **distancesRet, int *ndistancesRet) { - virNumaDistance *tmp = NULL; + g_autofree virNumaDistance *tmp = NULL; int tmp_size = 0; - int ret = -1; - int *distances = NULL; + g_autofree int *distances = NULL; int ndistances = 0; size_t i; if (virNumaGetDistances(node, &distances, &ndistances) < 0) - goto cleanup; + return -1; if (!distances) { *distancesRet = NULL; @@ -1521,11 +1520,8 @@ virCapabilitiesGetNUMADistances(int node, *ndistancesRet = tmp_size; *distancesRet = g_steal_pointer(&tmp); tmp_size = 0; - ret = 0; - cleanup: - VIR_FREE(distances); - VIR_FREE(tmp); - return ret; + + return 0; } static int @@ -1533,13 +1529,12 @@ virCapabilitiesGetNUMAPagesInfo(int node, virCapsHostNUMACellPageInfo **pageinfo, int *npageinfo) { - int ret = -1; - unsigned int *pages_size = NULL; - unsigned long long *pages_avail = NULL; + g_autofree unsigned int *pages_size = NULL; + g_autofree unsigned long long *pages_avail = NULL; size_t npages, i; if (virNumaGetPages(node, &pages_size, &pages_avail, NULL, &npages) < 0) - goto cleanup; + return -1; *pageinfo = g_new0(virCapsHostNUMACellPageInfo, npages); *npageinfo = npages; @@ -1549,12 +1544,7 @@ virCapabilitiesGetNUMAPagesInfo(int node, (*pageinfo)[i].avail = pages_avail[i]; } - ret = 0; - - cleanup: - VIR_FREE(pages_avail); - VIR_FREE(pages_size); - return ret; + return 0; } @@ -1685,7 +1675,7 @@ static int virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) { virNodeInfo nodeinfo; - virCapsHostNUMACellCPU *cpus; + g_autofree virCapsHostNUMACellCPU *cpus; int ncpus; int n, s, c, t; int id, cid; @@ -1741,7 +1731,6 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) error: for (; cid >= 0; cid--) virBitmapFree(cpus[cid].siblings); - VIR_FREE(cpus); return -1; } @@ -1925,7 +1914,7 @@ static int virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) { int n; - virCapsHostNUMACellCPU *cpus = NULL; + g_autofree virCapsHostNUMACellCPU *cpus = NULL; int ret = -1; int ncpus = 0; int max_node; @@ -1989,7 +1978,6 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) cleanup: virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); - VIR_FREE(cpus); return ret; } @@ -2034,22 +2022,18 @@ virCapabilitiesHostNUMANewHost(void) int virCapabilitiesInitPages(virCaps *caps) { - int ret = -1; - unsigned int *pages_size = NULL; + g_autofree unsigned int *pages_size = NULL; size_t npages; if (virNumaGetPages(-1 /* Magic constant for overall info */, &pages_size, NULL, NULL, &npages) < 0) - goto cleanup; + return -1; caps->host.pagesSize = g_steal_pointer(&pages_size); caps->host.nPagesSize = npages; npages = 0; - ret = 0; - cleanup: - VIR_FREE(pages_size); - return ret; + return 0; } -- 2.33.0
On a Monday in 2022, Jiang Jiacheng wrote:
Use g_autofree in capabilities.c for some pointers still using manual cleanup, and remove unnecessary cleanup.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/capabilities.c | 44 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 30 deletions(-)
@@ -1685,7 +1675,7 @@ static int virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) { virNodeInfo nodeinfo; - virCapsHostNUMACellCPU *cpus; + g_autofree virCapsHostNUMACellCPU *cpus;
All g_auto variable declarations must be initialized, otherwise we might try to free some uninitialized pointer. `ninja test` complains:
MALLOC_PERTURB_=46 /usr/bin/make -C /home/jtomko/build/libvirt/build-aux sc_require_attribute_cleanup_initialization ―――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――――――――――――――――― stdout: make: Entering directory '/home/jtomko/build/libvirt/build-aux' /home/jtomko/work/libvirt/src/conf/capabilities.c:1678: g_autofree virCapsHostNUMACellCPU *cpus; make: Leaving directory '/home/jtomko/build/libvirt/build-aux' stderr: variable declared with a cleanup macro must be initialized make: *** [/home/jtomko/work/libvirt/build-aux/syntax-check.mk:851: sc_require_attribute_cleanup_initialization] Error 1
int ncpus; int n, s, c, t; int id, cid; @@ -1741,7 +1731,6 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) error: for (; cid >= 0; cid--) virBitmapFree(cpus[cid].siblings); - VIR_FREE(cpus);
Mixing automatic cleanup of `cpus` with manual clearing of its content looks a bit confusing to me. virCapabilitiesClearHostNUMACellCPUTopology does not seem to be easily usable with g_auto, since `ncpus` is stored separately. Jano
return -1; }
Thanks for your review. So, it's may be better to don's use g_autofree if its content need to be freed manualy in cleanup? A similiar situation could be found in nwfilter_ebiptables_driver.c, function 'ebiptablesApplyNewRules' assigned subchains with g_autofree and freed its content using g_free in the cleanup. Should we unify those situation? On 2022/10/11 15:31, Ján Tomko wrote:
On a Monday in 2022, Jiang Jiacheng wrote:
Use g_autofree in capabilities.c for some pointers still using manual cleanup, and remove unnecessary cleanup.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/capabilities.c | 44 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 30 deletions(-)
@@ -1685,7 +1675,7 @@ static int virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) { virNodeInfo nodeinfo; - virCapsHostNUMACellCPU *cpus; + g_autofree virCapsHostNUMACellCPU *cpus;
All g_auto variable declarations must be initialized, otherwise we might try to free some uninitialized pointer.
I'm sorry about this mistake, I will fix it in next version.
`ninja test` complains:
MALLOC_PERTURB_=46 /usr/bin/make -C /home/jtomko/build/libvirt/build-aux sc_require_attribute_cleanup_initialization ―――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――――――――――――――――― stdout: make: Entering directory '/home/jtomko/build/libvirt/build-aux' /home/jtomko/work/libvirt/src/conf/capabilities.c:1678: g_autofree virCapsHostNUMACellCPU *cpus; make: Leaving directory '/home/jtomko/build/libvirt/build-aux' stderr: variable declared with a cleanup macro must be initialized make: *** [/home/jtomko/work/libvirt/build-aux/syntax-check.mk:851: sc_require_attribute_cleanup_initialization] Error 1
int ncpus; int n, s, c, t; int id, cid; @@ -1741,7 +1731,6 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps) error: for (; cid >= 0; cid--) virBitmapFree(cpus[cid].siblings); - VIR_FREE(cpus);
Mixing automatic cleanup of `cpus` with manual clearing of its content looks a bit confusing to me. virCapabilitiesClearHostNUMACellCPUTopology does not seem to be easily usable with g_auto, since `ncpus` is stored separately.
Jano
return -1; }
participants (2)
-
Jiang Jiacheng -
Ján Tomko