src/conf/capabilities.c | 44 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 30 deletions(-)
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; >> } >> >
© 2016 - 2024 Red Hat, Inc.