[PATCH] lib: Introduce and use g_autoptr() for virInterfaceDef

[Michal Prívozník]

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 at 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 at redhat.com>
> 

Pushed, thank you.

Michal