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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
Pushed, thank you.
Michal
