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@xxxxxxxxxx>
> ---
> 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.c b/src/conf/interface_conf.c
> index b45dc37379..f2b3804bec 100644
> --- a/src/conf/interface_conf.c
> +++ b/src/conf/interface_conf.c
> @@ -679,7 +679,7 @@ static virInterfaceDef *
> virInterfaceDefParseXML(xmlXPathContextPtr ctxt,
> int parentIfType)
> {
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
> int type;
> char *tmp;
> VIR_XPATH_NODE_AUTORESTORE(ctxt)
> @@ -716,28 +716,28 @@ virInterfaceDefParseXML(xmlXPathContextPtr
> ctxt,
> virReportError(VIR_ERR_XML_ERROR,
> _("interface has unsupported type '%s'"),
> virInterfaceTypeToString(type));
> - goto error;
> + return NULL;
> }
> def->type = type;
>
> if (virInterfaceDefParseName(def, ctxt) < 0)
> - goto error;
> + return NULL;
>
> if (parentIfType == VIR_INTERFACE_TYPE_LAST) {
> /* only recognize these in toplevel bond interfaces */
> if (virInterfaceDefParseStartMode(def, ctxt) < 0)
> - goto error;
> + return NULL;
> if (virInterfaceDefParseMtu(def, ctxt) < 0)
> - goto error;
> + return NULL;
> if (virInterfaceDefParseIfAdressing(def, ctxt) < 0)
> - goto error;
> + return NULL;
> }
>
> if (type != VIR_INTERFACE_TYPE_BRIDGE) {
> /* link status makes no sense for a bridge */
> lnk = virXPathNode("./link", ctxt);
> if (lnk && virInterfaceLinkParseXML(lnk, &def->lnk) < 0)
> - goto error;
> + return NULL;
> }
>
> switch (type) {
> @@ -751,11 +751,11 @@ virInterfaceDefParseXML(xmlXPathContextPtr
> ctxt,
> if (!(bridge = virXPathNode("./bridge[1]", ctxt))) {
> virReportError(VIR_ERR_XML_ERROR,
> "%s", _("bridge interface misses the
> bridge element"));
> - goto error;
> + return NULL;
> }
> ctxt->node = bridge;
> if (virInterfaceDefParseBridge(def, ctxt) < 0)
> - goto error;
> + return NULL;
> break;
> }
> case VIR_INTERFACE_TYPE_BOND: {
> @@ -764,11 +764,11 @@ virInterfaceDefParseXML(xmlXPathContextPtr
> ctxt,
> if (!(bond = virXPathNode("./bond[1]", ctxt))) {
> virReportError(VIR_ERR_XML_ERROR,
> "%s", _("bond interface misses the
> bond element"));
> - goto error;
> + return NULL;
> }
> ctxt->node = bond;
> if (virInterfaceDefParseBond(def, ctxt) < 0)
> - goto error;
> + return NULL;
> break;
> }
> case VIR_INTERFACE_TYPE_VLAN: {
> @@ -777,21 +777,17 @@ virInterfaceDefParseXML(xmlXPathContextPtr
> ctxt,
> if (!(vlan = virXPathNode("./vlan[1]", ctxt))) {
> virReportError(VIR_ERR_XML_ERROR,
> "%s", _("vlan interface misses the
> vlan element"));
> - goto error;
> + return NULL;
> }
> ctxt->node = vlan;
> if (virInterfaceDefParseVlan(def, ctxt) < 0)
> - goto error;
> + return NULL;
> break;
> }
>
> }
>
> - return def;
> -
> - error:
> - virInterfaceDefFree(def);
> - return NULL;
> + return g_steal_pointer(&def);
> }
>
>
> 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_netcf.c
> b/src/interface/interface_backend_netcf.c
> index 78fd4f9bc7..146a703953 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -387,7 +387,7 @@ static int
> netcfConnectNumOfInterfacesImpl(virConnectPtr conn,
> }
>
> for (i = 0; i < count; i++) {
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
> struct netcf_if *iface;
>
> iface = ncf_lookup_by_name(driver->netcf, names[i]);
> @@ -416,11 +416,8 @@ static int
> netcfConnectNumOfInterfacesImpl(virConnectPtr conn,
> }
> ncf_if_free(iface);
>
> - if (!filter(conn, def)) {
> - virInterfaceDefFree(def);
> + if (!filter(conn, def))
> continue;
> - }
> - virInterfaceDefFree(def);
>
> want++;
> }
> @@ -481,7 +478,7 @@ static int
> netcfConnectListInterfacesImpl(virConnectPtr conn,
> }
>
> for (i = 0; i < count && want < nnames; i++) {
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
> struct netcf_if *iface;
>
> iface = ncf_lookup_by_name(driver->netcf, allnames[i]);
> @@ -510,11 +507,8 @@ static int
> netcfConnectListInterfacesImpl(virConnectPtr conn,
> }
> ncf_if_free(iface);
>
> - if (!filter(conn, def)) {
> - virInterfaceDefFree(def);
> + if (!filter(conn, def))
> continue;
> - }
> - virInterfaceDefFree(def);
>
> names[want++] = g_steal_pointer(&allnames[i]);
> }
> @@ -665,7 +659,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
> tmp_iface_objs = g_new0(virInterfacePtr, count + 1);
>
> for (i = 0; i < count; i++) {
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> iface = ncf_lookup_by_name(driver->netcf, names[i]);
> if (!iface) {
> @@ -693,20 +687,17 @@ netcfConnectListAllInterfaces(virConnectPtr
> conn,
> if (!virConnectListAllInterfacesCheckACL(conn, def)) {
> ncf_if_free(iface);
> iface = NULL;
> - virInterfaceDefFree(def);
> continue;
> }
>
> if (ifaces) {
> - if (!(iface_obj = virGetInterface(conn, def->name, def-
> >mac))) {
> - virInterfaceDefFree(def);
> + if (!(iface_obj = virGetInterface(conn, def->name, def-
> >mac)))
> goto cleanup;
> - }
> +
> tmp_iface_objs[niface_objs] = iface_obj;
> }
> niface_objs++;
>
> - virInterfaceDefFree(def);
> ncf_if_free(iface);
> iface = NULL;
> }
> @@ -743,7 +734,7 @@ static virInterfacePtr
> netcfInterfaceLookupByName(virConnectPtr conn,
> {
> struct netcf_if *iface;
> virInterfacePtr ret = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> virObjectLock(driver);
> iface = ncf_lookup_by_name(driver->netcf, name);
> @@ -772,7 +763,6 @@ static virInterfacePtr
> netcfInterfaceLookupByName(virConnectPtr conn,
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -783,7 +773,7 @@ static virInterfacePtr
> netcfInterfaceLookupByMACString(virConnectPtr conn,
> struct netcf_if *iface;
> int niface;
> virInterfacePtr ret = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> virObjectLock(driver);
> niface = ncf_lookup_by_mac_string(driver->netcf, macstr, 1,
> &iface);
> @@ -820,7 +810,6 @@ static virInterfacePtr
> netcfInterfaceLookupByMACString(virConnectPtr conn,
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -830,7 +819,7 @@ static char
> *netcfInterfaceGetXMLDesc(virInterfacePtr ifinfo,
> {
> struct netcf_if *iface = NULL;
> char *xmlstr = NULL;
> - virInterfaceDef *ifacedef = NULL;
> + g_autoptr(virInterfaceDef) ifacedef = NULL;
> char *ret = NULL;
> bool active;
>
> @@ -880,7 +869,6 @@ static char
> *netcfInterfaceGetXMLDesc(virInterfacePtr ifinfo,
> cleanup:
> ncf_if_free(iface);
> VIR_FREE(xmlstr);
> - virInterfaceDefFree(ifacedef);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -891,7 +879,7 @@ static virInterfacePtr
> netcfInterfaceDefineXML(virConnectPtr conn,
> {
> struct netcf_if *iface = NULL;
> char *xmlstr = NULL;
> - virInterfaceDef *ifacedef = NULL;
> + g_autoptr(virInterfaceDef) ifacedef = NULL;
> virInterfacePtr ret = NULL;
>
> virCheckFlags(VIR_INTERFACE_DEFINE_VALIDATE, NULL);
> @@ -929,7 +917,6 @@ static virInterfacePtr
> netcfInterfaceDefineXML(virConnectPtr conn,
> cleanup:
> ncf_if_free(iface);
> VIR_FREE(xmlstr);
> - virInterfaceDefFree(ifacedef);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -937,7 +924,7 @@ static virInterfacePtr
> netcfInterfaceDefineXML(virConnectPtr conn,
> static int netcfInterfaceUndefine(virInterfacePtr ifinfo)
> {
> struct netcf_if *iface = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> int ret = -1;
>
> virObjectLock(driver);
> @@ -968,7 +955,6 @@ static int netcfInterfaceUndefine(virInterfacePtr
> ifinfo)
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -977,7 +963,7 @@ static int netcfInterfaceCreate(virInterfacePtr
> ifinfo,
> unsigned int flags)
> {
> struct netcf_if *iface = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> int ret = -1;
> bool active;
>
> @@ -1020,7 +1006,6 @@ static int netcfInterfaceCreate(virInterfacePtr
> ifinfo,
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -1029,7 +1014,7 @@ static int
> netcfInterfaceDestroy(virInterfacePtr ifinfo,
> unsigned int flags)
> {
> struct netcf_if *iface = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> int ret = -1;
> bool active;
>
> @@ -1072,7 +1057,6 @@ static int
> netcfInterfaceDestroy(virInterfacePtr ifinfo,
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> @@ -1080,7 +1064,7 @@ static int
> netcfInterfaceDestroy(virInterfacePtr ifinfo,
> static int netcfInterfaceIsActive(virInterfacePtr ifinfo)
> {
> struct netcf_if *iface = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> int ret = -1;
> bool active;
>
> @@ -1105,7 +1089,6 @@ static int
> netcfInterfaceIsActive(virInterfacePtr ifinfo)
>
> cleanup:
> ncf_if_free(iface);
> - virInterfaceDefFree(def);
> virObjectUnlock(driver);
> return ret;
> }
> 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
> @@ -165,7 +165,7 @@ udevNumOfInterfacesByStatus(virConnectPtr conn,
> virUdevStatus status,
> udev_list_entry_foreach(dev_entry, devices) {
> struct udev_device *dev;
> const char *path;
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> path = udev_list_entry_get_name(dev_entry);
> dev = udev_device_new_from_syspath(udev, path);
> @@ -174,7 +174,6 @@ udevNumOfInterfacesByStatus(virConnectPtr conn,
> virUdevStatus status,
> if (filter(conn, def))
> count++;
> udev_device_unref(dev);
> - virInterfaceDefFree(def);
> }
>
> cleanup:
> @@ -218,7 +217,7 @@ udevListInterfacesByStatus(virConnectPtr conn,
> udev_list_entry_foreach(dev_entry, devices) {
> struct udev_device *dev;
> const char *path;
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> /* Ensure we won't exceed the size of our array */
> if (count > names_len)
> @@ -233,7 +232,6 @@ udevListInterfacesByStatus(virConnectPtr conn,
> count++;
> }
> udev_device_unref(dev);
> - virInterfaceDefFree(def);
> }
>
> udev_enumerate_unref(enumerate);
> @@ -355,7 +353,7 @@ udevConnectListAllInterfaces(virConnectPtr conn,
> const char *path;
> const char *name;
> const char *macaddr;
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> path = udev_list_entry_get_name(dev_entry);
> dev = udev_device_new_from_syspath(udev, path);
> @@ -366,10 +364,8 @@ udevConnectListAllInterfaces(virConnectPtr conn,
> def = udevGetMinimalDefForDevice(dev);
> if (!virConnectListAllInterfacesCheckACL(conn, def)) {
> udev_device_unref(dev);
> - virInterfaceDefFree(def);
> continue;
> }
> - virInterfaceDefFree(def);
>
> /* Filter the results */
> if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
> @@ -413,7 +409,7 @@ udevInterfaceLookupByName(virConnectPtr conn,
> const char *name)
> struct udev *udev = udev_ref(driver->udev);
> struct udev_device *dev;
> virInterfacePtr ret = NULL;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
>
> /* get a device reference based on the device name */
> dev = udev_device_new_from_subsystem_sysname(udev, "net", name);
> @@ -435,7 +431,6 @@ udevInterfaceLookupByName(virConnectPtr conn,
> const char *name)
>
> cleanup:
> udev_unref(udev);
> - virInterfaceDefFree(def);
>
> return ret;
> }
> @@ -447,7 +442,7 @@ udevInterfaceLookupByMACString(virConnectPtr
> conn, const char *macstr)
> struct udev_enumerate *enumerate = NULL;
> struct udev_list_entry *dev_entry;
> struct udev_device *dev;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> virInterfacePtr ret = NULL;
>
> enumerate = udevGetDevices(udev, VIR_UDEV_IFACE_ALL);
> @@ -499,7 +494,6 @@ udevInterfaceLookupByMACString(virConnectPtr
> conn, const char *macstr)
> if (enumerate)
> udev_enumerate_unref(enumerate);
> udev_unref(udev);
> - virInterfaceDefFree(def);
>
> return ret;
> }
> @@ -945,7 +939,7 @@ static virInterfaceDef * ATTRIBUTE_NONNULL(1)
> udevGetIfaceDef(struct udev *udev, const char *name)
> {
> struct udev_device *dev = NULL;
> - virInterfaceDef *ifacedef;
> + g_autoptr(virInterfaceDef) ifacedef = NULL;
> unsigned int mtu;
> const char *mtu_str;
> char *vlan_parent_dev = NULL;
> @@ -1038,13 +1032,11 @@ udevGetIfaceDef(struct udev *udev, const char
> *name)
>
> udev_device_unref(dev);
>
> - return ifacedef;
> + return g_steal_pointer(&ifacedef);
>
> error:
> udev_device_unref(dev);
>
> - virInterfaceDefFree(ifacedef);
> -
> return NULL;
> }
>
> @@ -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@xxxxxxxxxx>
> cleanup:
> /* decrement our udev ptr */
> udev_unref(udev);
> @@ -1085,7 +1075,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
> {
> struct udev *udev = udev_ref(driver->udev);
> struct udev_device *dev;
> - virInterfaceDef *def = NULL;
> + g_autoptr(virInterfaceDef) def = NULL;
> int status = -1;
>
> dev = udev_device_new_from_subsystem_sysname(udev, "net",
> @@ -1110,7 +1100,6 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
>
> cleanup:
> udev_unref(udev);
> - virInterfaceDefFree(def);
>
> return status;
> }
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 13d07e570e..ea474d55ac 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1096,7 +1096,7 @@ testParseNetworks(testDriver *privconn,
> return -1;
>
> for (i = 0; i < num; i++) {
> - virNetworkDef *def;
> + g_autoptr(virNetworkDef) def = NULL;
> xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file,
> "network");
> if (!node)
> return -1;
> @@ -1105,10 +1105,9 @@ testParseNetworks(testDriver *privconn,
> if (!def)
> return -1;
>
> - if (!(obj = virNetworkObjAssignDef(privconn->networks, def,
> 0))) {
> - virNetworkDefFree(def);
> + if (!(obj = virNetworkObjAssignDef(privconn->networks, def,
> 0)))
> return -1;
> - }
> + def = NULL;
>
> virNetworkObjSetActive(obj, true);
> virNetworkObjEndAPI(&obj);
> @@ -1133,7 +1132,7 @@ testParseInterfaces(testDriver *privconn,
> return -1;
>
> for (i = 0; i < num; i++) {
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
> xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file,
> "interface");
> if (!node)
> @@ -1143,10 +1142,9 @@ testParseInterfaces(testDriver *privconn,
> if (!def)
> return -1;
>
> - if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces,
> def))) {
> - virInterfaceDefFree(def);
> + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces,
> def)))
> return -1;
> - }
> + def = NULL;
>
> virInterfaceObjSetActive(obj, true);
> virInterfaceObjEndAPI(&obj);
> @@ -6195,7 +6193,7 @@ testInterfaceDefineXML(virConnectPtr conn,
> unsigned int flags)
> {
> testDriver *privconn = conn->privateData;
> - virInterfaceDef *def;
> + g_autoptr(virInterfaceDef) def = NULL;
> virInterfaceObj *obj = NULL;
> virInterfaceDef *objdef;
> virInterfacePtr ret = NULL;
> @@ -6214,7 +6212,6 @@ testInterfaceDefineXML(virConnectPtr conn,
> ret = virGetInterface(conn, objdef->name, objdef->mac);
>
> cleanup:
> - virInterfaceDefFree(def);
> virInterfaceObjEndAPI(&obj);
> virObjectUnlock(privconn);
> return ret;
> diff --git a/tests/interfacexml2xmltest.c
> b/tests/interfacexml2xmltest.c
> index 8235e701a9..15fab88107 100644
> --- a/tests/interfacexml2xmltest.c
> +++ b/tests/interfacexml2xmltest.c
> @@ -18,28 +18,23 @@ testCompareXMLToXMLFiles(const char *xml)
> {
> g_autofree char *xmlData = NULL;
> g_autofree char *actual = NULL;
> - int ret = -1;
> - virInterfaceDef *dev = NULL;
> + g_autoptr(virInterfaceDef) dev = NULL;
>
> if (virTestLoadFile(xml, &xmlData) < 0)
> - goto fail;
> + return -1;
>
> if (!(dev = virInterfaceDefParseString(xmlData, 0)))
> - goto fail;
> + return -1;
>
> if (!(actual = virInterfaceDefFormat(dev)))
> - goto fail;
> + return -1;
>
> if (STRNEQ(xmlData, actual)) {
> virTestDifferenceFull(stderr, xmlData, xml, actual, NULL);
> - goto fail;
> + return -1;
> }
>
> - ret = 0;
> -
> - fail:
> - virInterfaceDefFree(dev);
> - return ret;
> + return 0;
> }
>
> static int
