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