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(-)
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@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;
}
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);
-
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
--
2.32.0
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@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@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
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@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@redhat.com>
>
Pushed, thank you.
Michal
© 2016 - 2024 Red Hat, Inc.