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;
>> }
>>
>