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

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(a)redhat.com&gt; > --- >  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).

> 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.

Otherwise: Reviewed-by: Tim Wiederhake <twiederh(a)redhat.com&gt;