On 11/2/21 10:04 AM, Tim Wiederhake wrote:
On Mon, 2021-11-01 at 16:25 +0100, Michal Privoznik wrote:
There are a lot of places where we call virInterfaceDefFree() explicitly. We can define autoptr cleanup macro and annotate declarations with g_autoptr() and remove plenty of those explicit free calls.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/interface_conf.c | 32 ++++++++--------- src/conf/interface_conf.h | 1 + src/conf/virinterfaceobj.c | 3 +- src/interface/interface_backend_netcf.c | 47 ++++++++--------------- -- src/interface/interface_backend_udev.c | 29 +++++---------- src/test/test_driver.c | 17 ++++----- tests/interfacexml2xmltest.c | 17 ++++----- 7 files changed, 53 insertions(+), 93 deletions(-)
diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index ea92e0fb31..510d83b2bf 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -153,6 +153,7 @@ struct _virInterfaceDef { void virInterfaceDefFree(virInterfaceDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virInterfaceDef, virInterfaceDefFree); virInterfaceDef * virInterfaceDefParseString(const char *xmlStr, diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 9439bb3d0b..ceb3ae7595 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -362,7 +362,7 @@ virInterfaceObjListCloneCb(void *payload, virInterfaceObj *srcObj = payload; struct _virInterfaceObjListCloneData *data = opaque; char *xml = NULL; - virInterfaceDef *backup = NULL; + g_autoptr(virInterfaceDef) backup = NULL; virInterfaceObj *obj; if (data->error) @@ -387,7 +387,6 @@ virInterfaceObjListCloneCb(void *payload, error: data->error = true; VIR_FREE(xml); - virInterfaceDefFree(backup); virObjectUnlock(srcObj); return 0; }
I believe there is a `g_steal_pointer` or similar missing in the call to `virInterfaceObjListAssignDef` (not shown in patch).
Actually, there's backup = NULL; missing right after successfull return from virInterfaceObjListAssignDef(); just like every other call has it (which can be then reworked to clear the pointer itself - will post a separate patch for that shortly). But good catch, thanks!
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 0217f16607..8c417714e5 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c
@@ -1053,7 +1045,7 @@ udevInterfaceGetXMLDesc(virInterfacePtr ifinfo, unsigned int flags) { struct udev *udev = udev_ref(driver->udev); - virInterfaceDef *ifacedef; + g_autoptr(virInterfaceDef) ifacedef = NULL; char *xmlstr = NULL; virCheckFlags(VIR_INTERFACE_XML_INACTIVE, NULL); @@ -1071,8 +1063,6 @@ udevInterfaceGetXMLDesc(virInterfacePtr ifinfo, xmlstr = virInterfaceDefFormat(ifacedef); - virInterfaceDefFree(ifacedef); -
This used to be a memory leak if the call to `virInterfaceGetXMLDescEnsureACL` (not shown in the patch) failed, isn't it? If so, we should mention that in the commit message.
Yes, let me add it there.
Otherwise: Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Pushed, thank you. Michal