Re: [PATCH] qemu: Add VIR_FREE in ADD_BITMAP

On 9/28/23 13:55, Anastasia Belova wrote: > virBitmapFormat returns the string that should be freed. > > All strings in three ADD_BITMAP calls in qemuDomainGetGuestVcpusParams > are contained in tmp. So memory leak is possible here without VIR_FREE. > > Fixes: 3e7db8d3e8 ("Remove backslash alignment attempts") Nothing in that commit suggests the VIR_FREE() was removed. In fact, digging in our history it was commit 0108deb944af5ca6f1da350c9d0352c8ed18738b which removed the call. Nit pick: I dislike short hashes because they are not unique. For instance, this short has is 10 characters, but as our history grows it'll need to grow too, otherwise it won't be unique. What is unique though (and both human and machine readable) is: 'git describe --tags' and if it's older commit then 'git describe --tags --contains'. But the purpose of 'Fixes' in our commit messages is to help downstream maintainers. For instance, when I'd be cherry picking that defect commit, plain search through git log shows what other commits fix it. Another reason against short hashes. > Signed-off-by: Anastasia Belova <abelova(a)astralinux.ru&gt; > --- > src/qemu/qemu_driver.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9e0f204e44..a70bfb6450 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -18420,6 +18420,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params, > goto cleanup; \ > if (virTypedParamsAddString(&par, &npar, &maxpar, #name, tmp) < 0) \ > goto cleanup; \ > + VIR_FREE(tmp); \

Nice catch! We can use this opportunity to: 1) drop the trailing '\' as we don't really need the macro continue on the next (empty) line, 2) drop the trailing ';' so that ... > > ADD_BITMAP(vcpus); > ADD_BITMAP(online); ... these calls can retain their semicolons (and in fact enforce them). Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; and pushed. Michal