Re: [PATCH] capabilities: use g_autofree in capabilities.c
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(a)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;
> }
>