1:01 p.m.
This third series of patches also modifies a few files in src/util
to use VIR_AUTOFREE and VIR_AUTOPTR for automatic freeing of memory
and get rid of some VIR_FREE macro invocations and *Free function
calls.
Sukrit Bhatnagar (32):
util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: iscsi: use VIR_AUTOPTR for aggregate types
util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar
types
util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
util: netdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: netdev: use VIR_AUTOPTR for aggregate types
util: socketaddr: define cleanup function using
VIR_DEFINE_AUTOPTR_FUNC
util: socketaddr: use VIR_AUTOFREE instead of VIR_FREE for scalar
types
util: socketaddr: use VIR_AUTOPTR for aggregate types
util: netdevip: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
util: netdevip: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: netdevip: use VIR_AUTOPTR for aggregate types
util: netdevmacvlan: define cleanup function using
VIR_DEFINE_AUTOPTR_FUNC
util: netdevmacvlan: use VIR_AUTOFREE instead of VIR_FREE for scalar
types
util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for
scalar types
util: netdevopenvswitch: use VIR_AUTOPTR for aggregate types
util: netdevtap: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: netdevveth: use VIR_AUTOFREE instead of VIR_FREE for scalar
types
util: netdevveth: use VIR_AUTOPTR for aggregate types
util: netlink: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
util: netlink: use VIR_AUTOPTR for aggregate types
util: numa: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: numa: use VIR_AUTOPTR for aggregate types
util: perf: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
util: perf: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: pidfile: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: process: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: process: use VIR_AUTOPTR for aggregate types
util: qemu: use VIR_AUTOFREE instead of VIR_FREE for scalar types
util: qemu: use VIR_AUTOPTR for aggregate types
src/util/viriscsi.c | 99 ++++------
src/util/virmacaddr.c | 6 +
src/util/virmacaddr.h | 4 +
src/util/virnetdev.c | 412 +++++++++++++++-------------------------
src/util/virnetdev.h | 4 +
src/util/virnetdevbridge.c | 45 ++---
src/util/virnetdevip.c | 157 ++++++---------
src/util/virnetdevip.h | 4 +
src/util/virnetdevmacvlan.c | 41 ++--
src/util/virnetdevopenvswitch.c | 131 +++++--------
src/util/virnetdevtap.c | 11 +-
src/util/virnetdevveth.c | 33 ++--
src/util/virnetlink.c | 62 +++---
src/util/virnuma.c | 107 ++++-------
src/util/virperf.c | 20 +-
src/util/virperf.h | 8 +-
src/util/virpidfile.c | 185 ++++++------------
src/util/virprocess.c | 68 +++----
src/util/virqemu.c | 50 ++---
src/util/virsocketaddr.c | 113 +++++------
src/util/virsocketaddr.h | 9 +-
21 files changed, 610 insertions(+), 959 deletions(-)
--
1.8.3.1
1:01 p.m.
New subject: [libvirt] [PATCH v1 01/32] util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/viriscsi.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 653b4fd..48f4106 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -88,7 +88,7 @@ virISCSIGetSession(const char *devpath,
.session = NULL,
.devpath = devpath,
};
- char *error = NULL;
+ VIR_AUTOFREE(char *) error = NULL;
int exitstatus = 0;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
@@ -109,7 +109,6 @@ virISCSIGetSession(const char *devpath,
NULLSTR(error));
cleanup:
- VIR_FREE(error);
virCommandFree(cmd);
return cbdata.session;
}
@@ -125,10 +124,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
int ret = IQN_ERROR;
- char *outbuf = NULL;
char *line = NULL;
- char *iface = NULL;
- char *iqn = NULL;
+ VIR_AUTOFREE(char *) outbuf = NULL;
+ VIR_AUTOFREE(char *) iface = NULL;
+ VIR_AUTOFREE(char *) iqn = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
@@ -197,9 +196,6 @@ virStorageBackendIQNFound(const char *initiatoriqn,
if (ret == IQN_MISSING)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
- VIR_FREE(iqn);
- VIR_FREE(iface);
- VIR_FREE(outbuf);
virCommandFree(cmd);
return ret;
@@ -216,7 +212,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
char **ifacename)
{
int ret = -1, exitstatus = -1;
- char *temp_ifacename;
+ VIR_AUTOFREE(char *) temp_ifacename = NULL;
virCommandPtr cmd = NULL;
if (virAsprintf(&temp_ifacename,
@@ -277,7 +273,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
cleanup:
virCommandFree(cmd);
- VIR_FREE(temp_ifacename);
if (ret != 0)
VIR_FREE(*ifacename);
return ret;
@@ -299,7 +294,7 @@ virISCSIConnection(const char *portal,
NULL
};
virCommandPtr cmd;
- char *ifacename = NULL;
+ VIR_AUTOFREE(char *) ifacename = NULL;
cmd = virCommandNewArgs(baseargv);
virCommandAddArgSet(cmd, extraargv);
@@ -339,7 +334,6 @@ virISCSIConnection(const char *portal,
cleanup:
virCommandFree(cmd);
- VIR_FREE(ifacename);
return ret;
}
@@ -390,15 +384,13 @@ virISCSIGetTargets(char **const groups,
void *data)
{
struct virISCSITargetList *list = data;
- char *target;
+ VIR_AUTOFREE(char *) target = NULL;
if (VIR_STRDUP(target, groups[1]) < 0)
return -1;
- if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) {
- VIR_FREE(target);
+ if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0)
return -1;
- }
return 0;
}
@@ -498,8 +490,7 @@ virISCSIScanTargets(const char *portal,
size_t *ntargets,
char ***targets)
{
- char *ifacename = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) ifacename = NULL;
if (ntargets)
*ntargets = 0;
@@ -522,10 +513,8 @@ virISCSIScanTargets(const char *portal,
}
}
- ret = virISCSIScanTargetsInternal(portal, ifacename,
+ return virISCSIScanTargetsInternal(portal, ifacename,
persist, ntargets, targets);
- VIR_FREE(ifacename);
- return ret;
}
--
1.8.3.1
8:20 a.m.
New subject: [libvirt] [PATCH v1 01/32] util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:16PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 02/32] util: iscsi: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/viriscsi.c | 68 +++++++++++++++++------------------------------------
1 file changed, 22 insertions(+), 46 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 48f4106..81404f8 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -91,7 +91,7 @@ virISCSIGetSession(const char *devpath,
VIR_AUTOFREE(char *) error = NULL;
int exitstatus = 0;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
+ VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode",
"session", NULL);
virCommandSetErrorBuffer(cmd, &error);
@@ -101,15 +101,13 @@ virISCSIGetSession(const char *devpath,
vars,
virISCSIExtractSession,
&cbdata, NULL, &exitstatus) < 0)
- goto cleanup;
+ return NULL;
if (cbdata.session == NULL && !probe)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find iscsiadm session: %s"),
NULLSTR(error));
- cleanup:
- virCommandFree(cmd);
return cbdata.session;
}
@@ -128,7 +126,7 @@ virStorageBackendIQNFound(const char *initiatoriqn,
VIR_AUTOFREE(char *) outbuf = NULL;
VIR_AUTOFREE(char *) iface = NULL;
VIR_AUTOFREE(char *) iqn = NULL;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+ VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
*ifacename = NULL;
@@ -196,7 +194,6 @@ virStorageBackendIQNFound(const char *initiatoriqn,
if (ret == IQN_MISSING)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
- virCommandFree(cmd);
return ret;
error:
@@ -213,7 +210,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
{
int ret = -1, exitstatus = -1;
VIR_AUTOFREE(char *) temp_ifacename = NULL;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (virAsprintf(&temp_ifacename,
"libvirt-iface-%08llx",
@@ -272,7 +269,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
ret = 0;
cleanup:
- virCommandFree(cmd);
if (ret != 0)
VIR_FREE(*ifacename);
return ret;
@@ -285,7 +281,6 @@ virISCSIConnection(const char *portal,
const char *target,
const char **extraargv)
{
- int ret = -1;
const char *const baseargv[] = {
ISCSIADM,
"--mode", "node",
@@ -293,7 +288,7 @@ virISCSIConnection(const char *portal,
"--targetname", target,
NULL
};
- virCommandPtr cmd;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) ifacename = NULL;
cmd = virCommandNewArgs(baseargv);
@@ -306,7 +301,7 @@ virISCSIConnection(const char *portal,
break;
case IQN_MISSING:
if (virStorageBackendCreateIfaceIQN(initiatoriqn, &ifacename) != 0)
- goto cleanup;
+ return -1;
/*
* iscsiadm doesn't let you send commands to the Interface IQN,
* unless you've first issued a 'sendtargets' command to the
@@ -317,25 +312,20 @@ virISCSIConnection(const char *portal,
*/
if (virISCSIScanTargetsInternal(portal, ifacename,
true, NULL, NULL) < 0)
- goto cleanup;
+ return -1;
break;
case IQN_ERROR:
default:
- goto cleanup;
+ return -1;
}
virCommandAddArgList(cmd, "--interface", ifacename, NULL);
}
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
-
- return ret;
+ return 0;
}
@@ -362,14 +352,12 @@ virISCSIConnectionLogout(const char *portal,
int
virISCSIRescanLUNs(const char *session)
{
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+ VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
"--mode", "session",
"-r", session,
"-R",
NULL);
- int ret = virCommandRun(cmd, NULL);
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
@@ -419,8 +407,7 @@ virISCSIScanTargetsInternal(const char *portal,
int vars[] = { 2 };
struct virISCSITargetList list;
size_t i;
- int ret = -1;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+ VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
"--mode", "discovery",
"--type",
"sendtargets",
"--portal", portal,
@@ -446,7 +433,7 @@ virISCSIScanTargetsInternal(const char *portal,
vars,
virISCSIGetTargets,
&list, NULL, NULL) < 0)
- goto cleanup;
+ return -1;
if (ntargetsret && targetsret) {
*ntargetsret = list.ntargets;
@@ -457,10 +444,7 @@ virISCSIScanTargetsInternal(const char *portal,
VIR_FREE(list.targets);
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -538,9 +522,8 @@ int
virISCSINodeNew(const char *portal,
const char *target)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int status;
- int ret = -1;
cmd = virCommandNewArgList(ISCSIADM,
"--mode", "node",
@@ -553,20 +536,17 @@ virISCSINodeNew(const char *portal,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed new node mode for target '%s'"),
target);
- goto cleanup;
+ return -1;
}
if (status != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("%s failed new mode for target '%s' with status
'%d'"),
ISCSIADM, target, status);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -576,9 +556,8 @@ virISCSINodeUpdate(const char *portal,
const char *name,
const char *value)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int status;
- int ret = -1;
cmd = virCommandNewArgList(ISCSIADM,
"--mode", "node",
@@ -594,11 +573,8 @@ virISCSINodeUpdate(const char *portal,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to update '%s' of node mode for target
'%s'"),
name, target);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
--
1.8.3.1
8:47 a.m.
New subject: [libvirt] [PATCH v1 02/32] util: iscsi: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:17PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/viriscsi.c | 68 +++++++++++++++++------------------------------------
1 file changed, 22 insertions(+), 46 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 48f4106..81404f8 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -91,7 +91,7 @@ virISCSIGetSession(const char *devpath,
VIR_AUTOFREE(char *) error = NULL;
int exitstatus = 0;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
+ VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode",
"session", NULL);
There are a few code misalignment issues like ^this. I'll fix them locally.
...
@@ -576,9 +556,8 @@ virISCSINodeUpdate(const char *portal,
const char *name,
const char *value)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
One tiny nitpick I realized only now is that we should probably start being
consistent in what order we declare the autoclean variables, IOW I think we
should group all the autoclean variables together so visually you can
immediately tell which variables are handled automatically and which aren't. It
was all fine across this file except for ^this single occurrence, I'll fix
that.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------
1 file changed, 15 insertions(+), 30 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index e46ac35..bf30d7c 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
int fd, /* control socket */
struct ifreq *ifr) /* pre-filled bridge name */
{
- char *path = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname)
< 0)
return -1;
@@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
if (virFileWriteStr(path, valuestr, 0) < 0) {
virReportSystemError(errno,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
} else {
unsigned long paramid;
@@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
} else {
virReportSystemError(EINVAL,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
unsigned long args[] = { paramid, value, 0, 0 };
ifr->ifr_data = (char*)&args;
if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
virReportSystemError(errno,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
}
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
+ return 0;
}
@@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
const char *paramname, /* sysfs param name */
unsigned long *value) /* current value */
{
- char *path = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
int ret = -1;
int fd = -1;
struct ifreq ifr;
@@ -180,7 +176,7 @@ static int virNetDevBridgeGet(const char *brname,
return -1;
if (virFileExists(path)) {
- char *valuestr;
+ VIR_AUTOFREE(char *) valuestr = NULL;
if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long),
&valuestr) < 0)
goto cleanup;
@@ -189,10 +185,8 @@ static int virNetDevBridgeGet(const char *brname,
virReportSystemError(EINVAL,
_("Unable to get bridge %s %s"),
brname, paramname);
- VIR_FREE(valuestr);
goto cleanup;
}
- VIR_FREE(valuestr);
} else {
struct __bridge_info info;
unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long)&info, 0, 0
};
@@ -221,7 +215,6 @@ static int virNetDevBridgeGet(const char *brname,
ret = 0;
cleanup:
VIR_FORCE_CLOSE(fd);
- VIR_FREE(path);
return ret;
}
#endif /* __linux__ */
@@ -233,7 +226,7 @@ virNetDevBridgePortSet(const char *brname,
const char *paramname,
unsigned long value)
{
- char *path = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
char valuestr[INT_BUFSIZE_BOUND(value)];
int ret = -1;
@@ -254,7 +247,6 @@ virNetDevBridgePortSet(const char *brname,
brname, ifname, paramname, valuestr);
}
- VIR_FREE(path);
return ret;
}
@@ -265,29 +257,24 @@ virNetDevBridgePortGet(const char *brname,
const char *paramname,
unsigned long *value)
{
- char *path = NULL;
- char *valuestr = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) valuestr = NULL;
if (virAsprintf(&path, SYSFS_NET_DIR "%s/brif/%s/%s",
brname, ifname, paramname) < 0)
return -1;
if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long), &valuestr) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) {
virReportSystemError(EINVAL,
_("Unable to get bridge %s port %s %s"),
brname, ifname, paramname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(path);
- VIR_FREE(valuestr);
- return ret;
+ return 0;
}
@@ -430,7 +417,7 @@ virNetDevBridgeCreate(const char *brname)
/* use a netlink RTM_NEWLINK message to create the bridge */
const char *type = "bridge";
int rc = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
unsigned int recvbuflen;
@@ -495,7 +482,6 @@ virNetDevBridgeCreate(const char *brname)
rc = 0;
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
unsigned int flags, bool isAdd)
{
int ret = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
unsigned int recvbuflen;
struct nl_msg *nl_msg;
@@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname,
ret = 0;
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return ret;
malformed_resp:
--
1.8.3.1
9:03 a.m.
New subject: [libvirt] [PATCH v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------
1 file changed, 15 insertions(+), 30 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index e46ac35..bf30d7c 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
int fd, /* control socket */
struct ifreq *ifr) /* pre-filled bridge name */
{
- char *path = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname,
paramname) < 0)
return -1;
@@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
if (virFileWriteStr(path, valuestr, 0) < 0) {
virReportSystemError(errno,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
} else {
unsigned long paramid;
@@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
} else {
virReportSystemError(EINVAL,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
unsigned long args[] = { paramid, value, 0, 0 };
ifr->ifr_data = (char*)&args;
if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
virReportSystemError(errno,
_("Unable to set bridge %s %s"), brname,
paramname);
- goto cleanup;
+ return -1;
}
}
- ret = 0;
- cleanup:
- VIR_FREE(path);
- return ret;
+ return 0;
}
@@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
const char *paramname, /* sysfs param name */
unsigned long *value) /* current value */
{
- char *path = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
Referring to my response to previous patch, I'll move ^this at the end of the
"declare" block (there are a few identical spots across the patch).
...
malformed_resp:
@@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char
*ifname,
unsigned int flags, bool isAdd)
{
int ret = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
unsigned int recvbuflen;
struct nl_msg *nl_msg;
So, I believe ^this external type can easily be turned into an autoclean
variant.
@@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac,
const char *ifname,
ret = 0;
cleanup:
nlmsg_free(nl_msg);
So that ^this would be done automatically.
Otherwise it the patch looks fine.
Erik
12:33 p.m.
New subject: [libvirt] [PATCH v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Thu, 2 Aug 2018 at 19:33, Erik Skultety <eskultet(a)redhat.com> wrote:
On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------
> 1 file changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index e46ac35..bf30d7c 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
> int fd, /* control socket */
> struct ifreq *ifr) /* pre-filled bridge name */
> {
> - char *path = NULL;
> - int ret = -1;
> + VIR_AUTOFREE(char *) path = NULL;
>
> if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname,
paramname) < 0)
> return -1;
> @@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
> if (virFileWriteStr(path, valuestr, 0) < 0) {
> virReportSystemError(errno,
> _("Unable to set bridge %s %s"), brname,
paramname);
> - goto cleanup;
> + return -1;
> }
> } else {
> unsigned long paramid;
> @@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
> } else {
> virReportSystemError(EINVAL,
> _("Unable to set bridge %s %s"), brname,
paramname);
> - goto cleanup;
> + return -1;
> }
> unsigned long args[] = { paramid, value, 0, 0 };
> ifr->ifr_data = (char*)&args;
> if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> virReportSystemError(errno,
> _("Unable to set bridge %s %s"), brname,
paramname);
> - goto cleanup;
> + return -1;
> }
> }
>
> - ret = 0;
> - cleanup:
> - VIR_FREE(path);
> - return ret;
> + return 0;
> }
>
>
> @@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
> const char *paramname, /* sysfs param name */
> unsigned long *value) /* current value */
> {
> - char *path = NULL;
> + VIR_AUTOFREE(char *) path = NULL;
Referring to my response to previous patch, I'll move ^this at the end of the
"declare" block (there are a few identical spots across the patch).
...
> malformed_resp:
> @@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char
*ifname,
> unsigned int flags, bool isAdd)
> {
> int ret = -1;
> - struct nlmsghdr *resp = NULL;
> + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> struct nlmsgerr *err;
> unsigned int recvbuflen;
> struct nl_msg *nl_msg;
So, I believe ^this external type can easily be turned into an autoclean
variant.
> @@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char
*ifname,
> ret = 0;
> cleanup:
> nlmsg_free(nl_msg);
So that ^this would be done automatically.
We would need to pass the nlmsg_free function in the cleanup attribute somehow.
So, shall we not use the VIR_AUTO macros and declare it with cleanup attribute
explicitly, or create a new type and a new Free wrapper for it to use
with out macros?
Otherwise it the patch looks fine.
Erik
2:25 a.m.
New subject: [libvirt] [PATCH v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Thu, Aug 02, 2018 at 11:03:16PM +0530, Sukrit Bhatnagar wrote:
On Thu, 2 Aug 2018 at 19:33, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOFREE macro for declaring scalar variables, majority
> > of the VIR_FREE calls can be dropped, which in turn leads to
> > getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> > src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------
> > 1 file changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > index e46ac35..bf30d7c 100644
> > --- a/src/util/virnetdevbridge.c
> > +++ b/src/util/virnetdevbridge.c
> > @@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
> > int fd, /* control socket */
> > struct ifreq *ifr) /* pre-filled bridge
name */
> > {
> > - char *path = NULL;
> > - int ret = -1;
> > + VIR_AUTOFREE(char *) path = NULL;
> >
> > if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname,
paramname) < 0)
> > return -1;
> > @@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
> > if (virFileWriteStr(path, valuestr, 0) < 0) {
> > virReportSystemError(errno,
> > _("Unable to set bridge %s %s"),
brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > } else {
> > unsigned long paramid;
> > @@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
> > } else {
> > virReportSystemError(EINVAL,
> > _("Unable to set bridge %s %s"),
brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > unsigned long args[] = { paramid, value, 0, 0 };
> > ifr->ifr_data = (char*)&args;
> > if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> > virReportSystemError(errno,
> > _("Unable to set bridge %s %s"),
brname, paramname);
> > - goto cleanup;
> > + return -1;
> > }
> > }
> >
> > - ret = 0;
> > - cleanup:
> > - VIR_FREE(path);
> > - return ret;
> > + return 0;
> > }
> >
> >
> > @@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
> > const char *paramname, /* sysfs param name */
> > unsigned long *value) /* current value */
> > {
> > - char *path = NULL;
> > + VIR_AUTOFREE(char *) path = NULL;
>
> Referring to my response to previous patch, I'll move ^this at the end of the
> "declare" block (there are a few identical spots across the patch).
>
> ...
>
> > malformed_resp:
> > @@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const
char *ifname,
> > unsigned int flags, bool isAdd)
> > {
> > int ret = -1;
> > - struct nlmsghdr *resp = NULL;
> > + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> > struct nlmsgerr *err;
> > unsigned int recvbuflen;
> > struct nl_msg *nl_msg;
>
> So, I believe ^this external type can easily be turned into an autoclean
> variant.
>
> > @@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const
char *ifname,
> > ret = 0;
> > cleanup:
> > nlmsg_free(nl_msg);
>
> So that ^this would be done automatically.
We would need to pass the nlmsg_free function in the cleanup attribute somehow.
So, shall we not use the VIR_AUTO macros and declare it with cleanup attribute
explicitly, or create a new type and a new Free wrapper for it to use
with out macros?
Why do you need a new Free wrapper? Naturally, you'd need a "single-token"
type
name, something like virNetlinkNlMsg which you'd put into src/util/virnetlink.h
along with the VIR_DEFINE_AUTOPTR_FUNC(virNetlinkNlMsg, nlmsg_free) and there
you go, you can use VIR_AUTOPTR with it.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virMacAddrPtr is declared using VIR_AUTOPTR,
the function virMacAddrFree will be run automatically on it when it
goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virmacaddr.c | 6 ++++++
src/util/virmacaddr.h | 4 ++++
2 files changed, 10 insertions(+)
diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 7afe032..e739775 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -252,3 +252,9 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
{
return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
}
+
+void
+virMacAddrFree(virMacAddrPtr addr)
+{
+ VIR_FREE(addr);
+}
diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
index d0dd4a4..39dd51b 100644
--- a/src/util/virmacaddr.h
+++ b/src/util/virmacaddr.h
@@ -25,6 +25,7 @@
# define __VIR_MACADDR_H__
# include "internal.h"
+# include "viralloc.h"
# define VIR_MAC_BUFLEN 6
# define VIR_MAC_HEXLEN (VIR_MAC_BUFLEN * 2)
@@ -64,5 +65,8 @@ int virMacAddrParseHex(const char* str,
bool virMacAddrIsUnicast(const virMacAddr *addr);
bool virMacAddrIsMulticast(const virMacAddr *addr);
bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
+void virMacAddrFree(virMacAddrPtr addr);
+
+VIR_DEFINE_AUTOPTR_FUNC(virMacAddr, virMacAddrFree)
#endif /* __VIR_MACADDR_H__ */
--
1.8.3.1
2:30 a.m.
New subject: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:19PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virMacAddrPtr is declared using VIR_AUTOPTR,
the function virMacAddrFree will be run automatically on it when it
goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virmacaddr.c | 6 ++++++
src/util/virmacaddr.h | 4 ++++
2 files changed, 10 insertions(+)
diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 7afe032..e739775 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -252,3 +252,9 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
{
return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
}
+
+void
+virMacAddrFree(virMacAddrPtr addr)
+{
+ VIR_FREE(addr);
+}
I understand the reason behind this change, however, I don't feel like this
will bring any benefits only because we said that VIR_AUTOFREE should be used
with scalar types only, I'd prefer simply using VIR_AUTOFREE here, CC'ng Pavel
to share his opinion.
Erik
2:34 a.m.
New subject: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Fri, Aug 03, 2018 at 09:30:22AM +0200, Erik Skultety wrote:
On Sat, Jul 28, 2018 at 11:31:19PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When a variable of type virMacAddrPtr is declared using VIR_AUTOPTR,
> the function virMacAddrFree will be run automatically on it when it
> goes out of scope.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virmacaddr.c | 6 ++++++
> src/util/virmacaddr.h | 4 ++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 7afe032..e739775 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -252,3 +252,9 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
> {
> return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
> }
> +
> +void
> +virMacAddrFree(virMacAddrPtr addr)
> +{
> + VIR_FREE(addr);
> +}
I understand the reason behind this change, however, I don't feel like this
will bring any benefits only because we said that VIR_AUTOFREE should be used
with scalar types only, I'd prefer simply using VIR_AUTOFREE here, CC'ng Pavel
to share his opinion.
Erik
+Pavel
2:51 a.m.
New subject: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Fri, Aug 03, 2018 at 09:34:51AM +0200, Erik Skultety wrote:
On Fri, Aug 03, 2018 at 09:30:22AM +0200, Erik Skultety wrote:
> On Sat, Jul 28, 2018 at 11:31:19PM +0530, Sukrit Bhatnagar wrote:
> > Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> > src/util/viralloc.h, define a new wrapper around an existing
> > cleanup function which will be called when a variable declared
> > with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> > viralloc.h include, since that has moved from the source module into
> > the header.
> >
> > When a variable of type virMacAddrPtr is declared using VIR_AUTOPTR,
> > the function virMacAddrFree will be run automatically on it when it
> > goes out of scope.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> > src/util/virmacaddr.c | 6 ++++++
> > src/util/virmacaddr.h | 4 ++++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> > index 7afe032..e739775 100644
> > --- a/src/util/virmacaddr.c
> > +++ b/src/util/virmacaddr.c
> > @@ -252,3 +252,9 @@ virMacAddrIsBroadcastRaw(const unsigned char
s[VIR_MAC_BUFLEN])
> > {
> > return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
> > }
> > +
> > +void
> > +virMacAddrFree(virMacAddrPtr addr)
> > +{
> > + VIR_FREE(addr);
> > +}
>
> I understand the reason behind this change, however, I don't feel like this
> will bring any benefits only because we said that VIR_AUTOFREE should be used
> with scalar types only, I'd prefer simply using VIR_AUTOFREE here, CC'ng
Pavel
> to share his opinion.
Sigh, I just realized we already have virPCIDeviceAddressFree which does the
very same thing (possibly more of those) which I missed during the review
of the previous batches. Oh well, we might have to go with this in the end in
order not to cause more confusion.
Erik
8:12 a.m.
New subject: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Fri, 2018-08-03 at 09:51 +0200, Erik Skultety wrote:
> > > +void
> > > +virMacAddrFree(virMacAddrPtr addr)
> > > +{
> > > + VIR_FREE(addr);
> > > +}
> >
> > I understand the reason behind this change, however, I don't feel like
this
> > will bring any benefits only because we said that VIR_AUTOFREE should be used
> > with scalar types only, I'd prefer simply using VIR_AUTOFREE here,
CC'ng Pavel
> > to share his opinion.
Sigh, I just realized we already have virPCIDeviceAddressFree which does the
very same thing (possibly more of those) which I missed during the review
of the previous batches. Oh well, we might have to go with this in the end in
order not to cause more confusion.
This kind of change is *very good*. Two main reasons for that:
* Consistency. Whenever you want to free a virSomething, you
should be able to just call virSomethingFree() without having
to look up whether or not virSomething contains dynamically
allocated memory.
* Extensibility. virMacAddrFree() might not do much *right now*,
but there's no guarantee that at some point down the line
virMacAddr won't need to allocate some memory[1], at which
point you'd have to introduce the function anyway as a
prerequisite for your actual changes, in addition of course
to tracking down all uses and convert them from VIR_AUTOFREE()
and VIR_FREE() to VIR_AUTOPTR() and virMacAddrFree().
I'd say that's well worth adding a few extra lines.
[1] Well, virMacAddr specifically probably won't :) But that's
definitely going to be the case for other types (and also
see the first point again :)
--
Andrea Bolognani / Red Hat / Virtualization
1:01 p.m.
New subject: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
and virNetDevMcastEntryFree, respectively, will be run
automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 9 ++++++++-
src/util/virnetdev.h | 4 ++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 0777eca..9eca786 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -29,7 +29,6 @@
#include "virfile.h"
#include "virerror.h"
#include "vircommand.h"
-#include "viralloc.h"
#include "virpci.h"
#include "virlog.h"
#include "virstring.h"
@@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
virMacAddr macaddr;
};
+static void
+virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
+{
+ VIR_FREE(entry);
+}
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
+
typedef struct _virNetDevMcastList virNetDevMcastList;
typedef virNetDevMcastList *virNetDevMcastListPtr;
struct _virNetDevMcastList {
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 71eaf45..8860ea1 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -30,6 +30,7 @@
# include "virmacaddr.h"
# include "virpci.h"
# include "virnetdevvlan.h"
+# include "viralloc.h"
# ifdef HAVE_STRUCT_IFREQ
typedef struct ifreq virIfreq;
@@ -313,4 +314,7 @@ int virNetDevSysfsFile(char **pf_sysfs_device_link,
int virNetDevRunEthernetScript(const char *ifname, const char *script)
ATTRIBUTE_NOINLINE;
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevRxFilter, virNetDevRxFilterFree)
+
#endif /* __VIR_NETDEV_H__ */
--
1.8.3.1
2:31 a.m.
New subject: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:20PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
and virNetDevMcastEntryFree, respectively, will be run
automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 9 ++++++++-
src/util/virnetdev.h | 4 ++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 0777eca..9eca786 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -29,7 +29,6 @@
#include "virfile.h"
#include "virerror.h"
#include "vircommand.h"
-#include "viralloc.h"
#include "virpci.h"
#include "virlog.h"
#include "virstring.h"
@@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
virMacAddr macaddr;
};
+static void
+virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
+{
+ VIR_FREE(entry);
+}
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
+
Same as with the previous patch, I'm not convinced by this change.
typedef struct _virNetDevMcastList virNetDevMcastList;
typedef virNetDevMcastList *virNetDevMcastListPtr;
struct _virNetDevMcastList {
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 71eaf45..8860ea1 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -30,6 +30,7 @@
# include "virmacaddr.h"
# include "virpci.h"
# include "virnetdevvlan.h"
+# include "viralloc.h"
# ifdef HAVE_STRUCT_IFREQ
typedef struct ifreq virIfreq;
@@ -313,4 +314,7 @@ int virNetDevSysfsFile(char **pf_sysfs_device_link,
int virNetDevRunEthernetScript(const char *ifname, const char *script)
ATTRIBUTE_NOINLINE;
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevRxFilter, virNetDevRxFilterFree)
+
To ^this hunk though:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
#endif /* __VIR_NETDEV_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:14 a.m.
New subject: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:20PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
and virNetDevMcastEntryFree, respectively, will be run
automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 9 ++++++++-
src/util/virnetdev.h | 4 ++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 0777eca..9eca786 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -29,7 +29,6 @@
#include "virfile.h"
#include "virerror.h"
#include "vircommand.h"
-#include "viralloc.h"
#include "virpci.h"
#include "virlog.h"
#include "virstring.h"
@@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
virMacAddr macaddr;
};
+static void
+virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
+{
+ VIR_FREE(entry);
+}
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
This causes a compile problem on non-linux platforms due to the function
being unused:
util/virnetdev.c:128:1: error: unused function 'virNetDevMcastEntryAutoPtrFree'
[-Werror,-Wunused-function]
VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
^
./util/viralloc.h:612:24: note: expanded from macro 'VIR_DEFINE_AUTOPTR_FUNC'
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
^
./util/viralloc.h:600:38: note: expanded from macro 'VIR_AUTOPTR_FUNC_NAME'
# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
^
<scratch space>:21:1: note: expanded from here
virNetDevMcastEntryAutoPtrFree
^
1 error generated.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:04 a.m.
New subject: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Tue, Aug 07, 2018 at 04:14:06PM +0100, Daniel P. Berrangé wrote:
On Sat, Jul 28, 2018 at 11:31:20PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
> are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
> and virNetDevMcastEntryFree, respectively, will be run
> automatically on them when they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virnetdev.c | 9 ++++++++-
> src/util/virnetdev.h | 4 ++++
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 0777eca..9eca786 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -29,7 +29,6 @@
> #include "virfile.h"
> #include "virerror.h"
> #include "vircommand.h"
> -#include "viralloc.h"
> #include "virpci.h"
> #include "virlog.h"
> #include "virstring.h"
> @@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
> virMacAddr macaddr;
> };
>
> +static void
> +virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
> +{
> + VIR_FREE(entry);
> +}
> +
> +VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
This causes a compile problem on non-linux platforms due to the function
being unused:
util/virnetdev.c:128:1: error: unused function 'virNetDevMcastEntryAutoPtrFree'
[-Werror,-Wunused-function]
VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
^
./util/viralloc.h:612:24: note: expanded from macro 'VIR_DEFINE_AUTOPTR_FUNC'
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
^
./util/viralloc.h:600:38: note: expanded from macro 'VIR_AUTOPTR_FUNC_NAME'
# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
^
<scratch space>:21:1: note: expanded from here
virNetDevMcastEntryAutoPtrFree
^
1 error generated.
Sigh...yeah, there are a few of them, I had some notes in patches that actually
used the definitions, nevertheless should have been compilable on their own
with clang in the first place. I'm already running my local fix in Travis, I'll
be sending patches afterwards.
Erik
11:06 a.m.
New subject: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Tue, Aug 07, 2018 at 06:04:37PM +0200, Erik Skultety wrote:
On Tue, Aug 07, 2018 at 04:14:06PM +0100, Daniel P. Berrangé wrote:
> On Sat, Jul 28, 2018 at 11:31:20PM +0530, Sukrit Bhatnagar wrote:
> > Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> > src/util/viralloc.h, define a new wrapper around an existing
> > cleanup function which will be called when a variable declared
> > with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> > viralloc.h include, since that has moved from the source module into
> > the header.
> >
> > When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
> > are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
> > and virNetDevMcastEntryFree, respectively, will be run
> > automatically on them when they go out of scope.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> > src/util/virnetdev.c | 9 ++++++++-
> > src/util/virnetdev.h | 4 ++++
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> > index 0777eca..9eca786 100644
> > --- a/src/util/virnetdev.c
> > +++ b/src/util/virnetdev.c
> > @@ -29,7 +29,6 @@
> > #include "virfile.h"
> > #include "virerror.h"
> > #include "vircommand.h"
> > -#include "viralloc.h"
> > #include "virpci.h"
> > #include "virlog.h"
> > #include "virstring.h"
> > @@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
> > virMacAddr macaddr;
> > };
> >
> > +static void
> > +virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
> > +{
> > + VIR_FREE(entry);
> > +}
> > +
> > +VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
>
> This causes a compile problem on non-linux platforms due to the function
> being unused:
>
> util/virnetdev.c:128:1: error: unused function
'virNetDevMcastEntryAutoPtrFree' [-Werror,-Wunused-function]
> VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
> ^
> ./util/viralloc.h:612:24: note: expanded from macro
'VIR_DEFINE_AUTOPTR_FUNC'
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> ^
> ./util/viralloc.h:600:38: note: expanded from macro 'VIR_AUTOPTR_FUNC_NAME'
> # define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
> ^
> <scratch space>:21:1: note: expanded from here
> virNetDevMcastEntryAutoPtrFree
> ^
> 1 error generated.
Sigh...yeah, there are a few of them, I had some notes in patches that actually
used the definitions, nevertheless should have been compilable on their own
with clang in the first place. I'm already running my local fix in Travis, I'll
be sending patches afterwards.
Worth checking a mingw build too, since mingw disables some stuff that
is still present on OS-X, so might tickle further code path variations.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
1:01 p.m.
New subject: [libvirt] [PATCH v1 06/32] util: netdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 276 ++++++++++++++++++---------------------------------
1 file changed, 96 insertions(+), 180 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 9eca786..7653f8b 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -535,10 +535,9 @@ int virNetDevSetMTUFromDevice(const char *ifname,
*/
int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
{
- int ret = -1;
- char *pid = NULL;
- char *phy = NULL;
- char *phy_path = NULL;
+ VIR_AUTOFREE(char *) pid = NULL;
+ VIR_AUTOFREE(char *) phy = NULL;
+ VIR_AUTOFREE(char *) phy_path = NULL;
int len;
if (virAsprintf(&pid, "%lld", (long long) pidInNs) == -1)
@@ -546,7 +545,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
/* The 802.11 wireless devices only move together with their PHY. */
if (virNetDevSysfsFile(&phy_path, ifname, "phy80211/name") < 0)
- goto cleanup;
+ return -1;
if ((len = virFileReadAllQuiet(phy_path, 1024, &phy)) <= 0) {
/* Not a wireless device. */
@@ -556,7 +555,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
argv[5] = pid;
if (virRun(argv, NULL) < 0)
- goto cleanup;
+ return -1;
} else {
const char *argv[] = {
@@ -569,15 +568,10 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
argv[2] = phy;
argv[5] = pid;
if (virRun(argv, NULL) < 0)
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(phy_path);
- VIR_FREE(phy);
- VIR_FREE(pid);
- return ret;
+ return 0;
}
#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ)
@@ -969,25 +963,21 @@ int virNetDevGetIndex(const char *ifname ATTRIBUTE_UNUSED,
int
virNetDevGetMaster(const char *ifname, char **master)
{
- int ret = -1;
- void *nlData = NULL;
+ VIR_AUTOFREE(void *) nlData = NULL;
struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
*master = NULL;
if (virNetlinkDumpLink(ifname, -1, &nlData, tb, 0, 0) < 0)
- goto cleanup;
+ return -1;
if (tb[IFLA_MASTER]) {
if (!(*master = virNetDevGetName(*(int *)RTA_DATA(tb[IFLA_MASTER]))))
- goto cleanup;
+ return -1;
}
VIR_DEBUG("IFLA_MASTER for %s is %s", ifname, *master ? *master :
"(none)");
- ret = 0;
- cleanup:
- VIR_FREE(nlData);
- return ret;
+ return 0;
}
@@ -1168,37 +1158,31 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char
*ifname,
static bool
virNetDevIsPCIDevice(const char *devpath)
{
- char *subsys_link = NULL;
- char *abs_path = NULL;
+ VIR_AUTOFREE(char *) subsys_link = NULL;
+ VIR_AUTOFREE(char *) abs_path = NULL;
char *subsys = NULL;
- bool ret = false;
if (virAsprintf(&subsys_link, "%s/subsystem", devpath) < 0)
return false;
if (!virFileExists(subsys_link))
- goto cleanup;
+ return false;
if (virFileResolveLink(subsys_link, &abs_path) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to resolve device subsystem symlink %s"),
subsys_link);
- goto cleanup;
+ return false;
}
subsys = last_component(abs_path);
- ret = STRPREFIX(subsys, "pci");
-
- cleanup:
- VIR_FREE(subsys_link);
- VIR_FREE(abs_path);
- return ret;
+ return STRPREFIX(subsys, "pci");
}
static virPCIDevicePtr
virNetDevGetPCIDevice(const char *devName)
{
- char *vfSysfsDevicePath = NULL;
+ VIR_AUTOFREE(char *) vfSysfsDevicePath = NULL;
virPCIDeviceAddressPtr vfPCIAddr = NULL;
virPCIDevicePtr vfPCIDevice = NULL;
@@ -1216,7 +1200,6 @@ virNetDevGetPCIDevice(const char *devName)
vfPCIAddr->slot, vfPCIAddr->function);
cleanup:
- VIR_FREE(vfSysfsDevicePath);
VIR_FREE(vfPCIAddr);
return vfPCIDevice;
@@ -1241,25 +1224,20 @@ int
virNetDevGetPhysPortID(const char *ifname,
char **physPortID)
{
- int ret = -1;
- char *physPortIDFile = NULL;
+ VIR_AUTOFREE(char *) physPortIDFile = NULL;
*physPortID = NULL;
if (virNetDevSysfsFile(&physPortIDFile, ifname, "phys_port_id") <
0)
- goto cleanup;
+ return -1;
/* a failure to read just means the driver doesn't support
- * phys_port_id, so set success now and ignore the return from
- * virFileReadAllQuiet().
+ * phys_port_id, so ignore the return from virFileReadAllQuiet()
+ * and return success.
*/
- ret = 0;
-
ignore_value(virFileReadAllQuiet(physPortIDFile, 1024, physPortID));
- cleanup:
- VIR_FREE(physPortIDFile);
- return ret;
+ return 0;
}
@@ -1282,10 +1260,11 @@ virNetDevGetVirtualFunctions(const char *pfname,
{
int ret = -1;
size_t i;
- char *pf_sysfs_device_link = NULL;
- char *pci_sysfs_device_link = NULL;
- char *pciConfigAddr = NULL;
- char *pfPhysPortID = NULL;
+ VIR_AUTOFREE(char *) pf_sysfs_device_link = NULL;
+ VIR_AUTOFREE(char *) pci_sysfs_device_link = NULL;
+ VIR_AUTOFREE(char *) pciConfigAddr = NULL;
+ VIR_AUTOFREE(char *) pfPhysPortID = NULL;
+ VIR_AUTOFREE(char **) tempVfname = NULL;
*virt_fns = NULL;
*n_vfname = 0;
@@ -1333,13 +1312,9 @@ virNetDevGetVirtualFunctions(const char *pfname,
cleanup:
if (ret < 0) {
- VIR_FREE(*vfname);
+ VIR_STEAL_PTR(tempVfname, *vfname);
VIR_FREE(*virt_fns);
}
- VIR_FREE(pfPhysPortID);
- VIR_FREE(pf_sysfs_device_link);
- VIR_FREE(pci_sysfs_device_link);
- VIR_FREE(pciConfigAddr);
return ret;
}
@@ -1355,17 +1330,12 @@ virNetDevGetVirtualFunctions(const char *pfname,
int
virNetDevIsVirtualFunction(const char *ifname)
{
- char *if_sysfs_device_link = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) if_sysfs_device_link = NULL;
if (virNetDevSysfsFile(&if_sysfs_device_link, ifname, "device") <
0)
- return ret;
+ return -1;
- ret = virPCIIsVirtualFunction(if_sysfs_device_link);
-
- VIR_FREE(if_sysfs_device_link);
-
- return ret;
+ return virPCIIsVirtualFunction(if_sysfs_device_link);
}
/**
@@ -1383,25 +1353,19 @@ int
virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname,
int *vf_index)
{
- char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) pf_sysfs_device_link = NULL;
+ VIR_AUTOFREE(char *) vf_sysfs_device_link = NULL;
if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") <
0)
- return ret;
+ return -1;
- if (virNetDevSysfsFile(&vf_sysfs_device_link, vfname, "device") < 0)
{
- VIR_FREE(pf_sysfs_device_link);
- return ret;
- }
+ if (virNetDevSysfsFile(&vf_sysfs_device_link, vfname, "device") <
0)
+ return -1;
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_device_link,
+ return virPCIGetVirtualFunctionIndex(pf_sysfs_device_link,
vf_sysfs_device_link,
vf_index);
- VIR_FREE(pf_sysfs_device_link);
- VIR_FREE(vf_sysfs_device_link);
-
- return ret;
}
/**
@@ -1417,19 +1381,18 @@ virNetDevGetVirtualFunctionIndex(const char *pfname, const char
*vfname,
int
virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
{
- char *physfn_sysfs_path = NULL;
- char *vfPhysPortID = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) physfn_sysfs_path = NULL;
+ VIR_AUTOFREE(char *) vfPhysPortID = NULL;
if (virNetDevGetPhysPortID(ifname, &vfPhysPortID) < 0)
- goto cleanup;
+ return -1;
if (virNetDevSysfsDeviceFile(&physfn_sysfs_path, ifname, "physfn") <
0)
- goto cleanup;
+ return -1;
if (virPCIGetNetName(physfn_sysfs_path, 0,
vfPhysPortID, pfname) < 0) {
- goto cleanup;
+ return -1;
}
if (!*pfname) {
@@ -1439,14 +1402,10 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The PF device for VF %s has no network device
name"),
ifname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(vfPhysPortID);
- VIR_FREE(physfn_sysfs_path);
- return ret;
+ return 0;
}
@@ -1470,26 +1429,25 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
int
virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
{
- char *virtfnName = NULL;
- char *virtfnSysfsPath = NULL;
- char *pfPhysPortID = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) virtfnName = NULL;
+ VIR_AUTOFREE(char *) virtfnSysfsPath = NULL;
+ VIR_AUTOFREE(char *) pfPhysPortID = NULL;
/* a VF may have multiple "ports", each one having its own netdev,
* and each netdev having a different phys_port_id. Be sure we get
* the VF netdev with a phys_port_id matchine that of pfname
*/
if (virNetDevGetPhysPortID(pfname, &pfPhysPortID) < 0)
- goto cleanup;
+ return -1;
if (virAsprintf(&virtfnName, "virtfn%d", vf) < 0)
- goto cleanup;
+ return -1;
/* this provides the path to the VF's directory in sysfs,
* e.g. "/sys/class/net/enp2s0f0/virtfn3"
*/
if (virNetDevSysfsDeviceFile(&virtfnSysfsPath, pfname, virtfnName) < 0)
- goto cleanup;
+ return -1;
/* and this gets the netdev name associated with it, which is a
* directory entry in [virtfnSysfsPath]/net,
@@ -1498,14 +1456,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
* isn't bound to a netdev driver, it won't have a netdev name,
* and vfname will be NULL).
*/
- ret = virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
-
- cleanup:
- VIR_FREE(virtfnName);
- VIR_FREE(virtfnSysfsPath);
- VIR_FREE(pfPhysPortID);
-
- return ret;
+ return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
}
@@ -1522,13 +1473,14 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) pf_sysfs_path = NULL;
+ VIR_AUTOFREE(char *) vf_sysfs_path = NULL;
+ VIR_AUTOFREE(char *) tempPfname = NULL;
*pfname = NULL;
if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
- return ret;
+ return -1;
if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
goto cleanup;
@@ -1536,16 +1488,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char
**pfname,
if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
goto cleanup;
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
+ return virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
cleanup:
- if (ret < 0)
- VIR_FREE(*pfname);
+ VIR_STEAL_PTR(tempPfname, *pfname);
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
-
- return ret;
+ return -1;
}
#else /* !__linux__ */
@@ -1657,7 +1605,7 @@ virNetDevSetVfConfig(const char *ifname, int vf,
{
int rc = -1;
char macstr[VIR_MAC_STRING_BUFLEN];
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
unsigned int recvbuflen = 0;
struct nl_msg *nl_msg;
@@ -1769,7 +1717,6 @@ virNetDevSetVfConfig(const char *ifname, int vf,
vlanid, rc < 0 ? "Fail" : "Success");
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -1843,19 +1790,15 @@ virNetDevGetVfConfig(const char *ifname, int vf, virMacAddrPtr
mac,
int *vlanid)
{
int rc = -1;
- void *nlData = NULL;
+ VIR_AUTOFREE(void *) nlData = NULL;
struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
int ifindex = -1;
rc = virNetlinkDumpLink(ifname, ifindex, &nlData, tb, 0, 0);
if (rc < 0)
- goto cleanup;
+ return rc;
- rc = virNetDevParseVfConfig(tb, vf, mac, vlanid);
-
- cleanup:
- VIR_FREE(nlData);
- return rc;
+ return virNetDevParseVfConfig(tb, vf, mac, vlanid);
}
@@ -1914,13 +1857,13 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
{
int ret = -1;
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
+ VIR_AUTOFREE(char *) pfDevOrig = NULL;
+ VIR_AUTOFREE(char *) vfDevOrig = NULL;
+ VIR_AUTOFREE(char *) filePath = NULL;
+ VIR_AUTOFREE(char *) fileStr = NULL;
virMacAddr oldMAC;
char MACStr[VIR_MAC_STRING_BUFLEN];
int oldVlanTag = -1;
- char *filePath = NULL;
- char *fileStr = NULL;
virJSONValuePtr configJSON = NULL;
if (vf >= 0) {
@@ -2030,10 +1973,6 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
ret = 0;
cleanup:
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
- VIR_FREE(filePath);
- VIR_FREE(fileStr);
virJSONValueFree(configJSON);
return ret;
}
@@ -2069,10 +2008,10 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
{
int ret = -1;
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
- char *filePath = NULL;
- char *fileStr = NULL;
+ VIR_AUTOFREE(char *) pfDevOrig = NULL;
+ VIR_AUTOFREE(char *) vfDevOrig = NULL;
+ VIR_AUTOFREE(char *) filePath = NULL;
+ VIR_AUTOFREE(char *) fileStr = NULL;
virJSONValuePtr configJSON = NULL;
const char *MACStr = NULL;
const char *adminMACStr = NULL;
@@ -2245,10 +2184,6 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
VIR_FREE(*vlan);
}
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
- VIR_FREE(filePath);
- VIR_FREE(fileStr);
virJSONValueFree(configJSON);
return ret;
}
@@ -2282,8 +2217,8 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
int ret = -1;
char MACStr[VIR_MAC_STRING_BUFLEN];
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
+ VIR_AUTOFREE(char *) pfDevOrig = NULL;
+ VIR_AUTOFREE(char *) vfDevOrig = NULL;
int vlanTag = -1;
virPCIDevicePtr vfPCIDevice = NULL;
@@ -2462,8 +2397,6 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
ret = 0;
cleanup:
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
virPCIDeviceFree(vfPCIDevice);
return ret;
}
@@ -2543,28 +2476,27 @@ int
virNetDevGetLinkInfo(const char *ifname,
virNetDevIfLinkPtr lnk)
{
- int ret = -1;
- char *path = NULL;
- char *buf = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
char *tmp;
int tmp_state;
unsigned int tmp_speed;
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0) {
virReportSystemError(errno,
_("unable to read: %s"),
path);
- goto cleanup;
+ return -1;
}
if (!(tmp = strchr(buf, '\n'))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
*tmp = '\0';
@@ -2575,7 +2507,7 @@ virNetDevGetLinkInfo(const char *ifname,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
lnk->state = tmp_state;
@@ -2586,26 +2518,24 @@ virNetDevGetLinkInfo(const char *ifname,
* speed if that's the case. */
if (lnk->state != VIR_NETDEV_IF_STATE_UP) {
lnk->speed = 0;
- ret = 0;
- goto cleanup;
+ return 0;
}
VIR_FREE(path);
VIR_FREE(buf);
if (virNetDevSysfsFile(&path, ifname, "speed") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAllQuiet(path, 1024, &buf) < 0) {
/* Some devices doesn't report speed, in which case we get EINVAL */
- if (errno == EINVAL) {
- ret = 0;
- goto cleanup;
- }
+ if (errno == EINVAL)
+ return 0;
+
virReportSystemError(errno,
_("unable to read: %s"),
path);
- goto cleanup;
+ return -1;
}
if (virStrToLong_ui(buf, &tmp, 10, &tmp_speed) < 0 ||
@@ -2613,16 +2543,12 @@ virNetDevGetLinkInfo(const char *ifname,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
lnk->speed = tmp_speed;
- ret = 0;
- cleanup:
- VIR_FREE(buf);
- VIR_FREE(path);
- return ret;
+ return 0;
}
#else
@@ -2830,7 +2756,7 @@ static int virNetDevGetMcastList(const char *ifname,
virNetDevMcastListPtr mcast)
{
char *cur = NULL;
- char *buf = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
char *next = NULL;
int ret = -1, len;
virNetDevMcastEntryPtr entry = NULL;
@@ -2866,7 +2792,6 @@ static int virNetDevGetMcastList(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(buf);
VIR_FREE(entry);
return ret;
@@ -3006,10 +2931,8 @@ static int
virNetDevRDMAFeature(const char *ifname,
virBitmapPtr *out)
{
- char *eth_devpath = NULL;
- char *ib_devpath = NULL;
- char *eth_res_buf = NULL;
- char *ib_res_buf = NULL;
+ VIR_AUTOFREE(char *) eth_devpath = NULL;
+ VIR_AUTOFREE(char *) eth_res_buf = NULL;
DIR *dirp = NULL;
struct dirent *dp;
int ret = -1;
@@ -3028,6 +2951,9 @@ virNetDevRDMAFeature(const char *ifname,
goto cleanup;
while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) {
+ VIR_AUTOFREE(char *) ib_devpath = NULL;
+ VIR_AUTOFREE(char *) ib_res_buf = NULL;
+
if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR
"%s/device/resource",
dp->d_name) < 0)
continue;
@@ -3036,17 +2962,11 @@ virNetDevRDMAFeature(const char *ifname,
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA));
break;
}
- VIR_FREE(ib_devpath);
- VIR_FREE(ib_res_buf);
}
ret = 0;
cleanup:
VIR_DIR_CLOSE(dirp);
- VIR_FREE(eth_devpath);
- VIR_FREE(ib_devpath);
- VIR_FREE(eth_res_buf);
- VIR_FREE(ib_res_buf);
return ret;
}
@@ -3205,7 +3125,7 @@ static uint32_t
virNetDevGetFamilyId(const char *family_name)
{
struct nl_msg *nl_msg = NULL;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct genlmsghdr* gmsgh = NULL;
struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, };
unsigned int recvbuflen;
@@ -3245,7 +3165,6 @@ virNetDevGetFamilyId(const char *family_name)
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return family_id;
}
@@ -3265,13 +3184,13 @@ virNetDevSwitchdevFeature(const char *ifname,
virBitmapPtr *out)
{
struct nl_msg *nl_msg = NULL;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
unsigned int recvbuflen;
struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
virPCIDevicePtr pci_device_ptr = NULL;
struct genlmsghdr* gmsgh = NULL;
const char *pci_name;
- char *pfname = NULL;
+ VIR_AUTOFREE(char *) pfname = NULL;
int is_vf = -1;
int ret = -1;
uint32_t family_id;
@@ -3333,8 +3252,6 @@ virNetDevSwitchdevFeature(const char *ifname,
cleanup:
nlmsg_free(nl_msg);
virPCIDeviceFree(pci_device_ptr);
- VIR_FREE(resp);
- VIR_FREE(pfname);
return ret;
}
# else
@@ -3375,7 +3292,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
int fd,
struct ifreq *ifr)
{
- struct ethtool_gfeatures *g_cmd;
+ VIR_AUTOFREE(struct ethtool_gfeatures *) g_cmd = NULL;
if (VIR_ALLOC_VAR(g_cmd,
struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
@@ -3385,7 +3302,6 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
g_cmd->size = GFEATURES_SIZE;
if (virNetDevGFeatureAvailable(fd, ifr, g_cmd))
ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL));
- VIR_FREE(g_cmd);
return 0;
}
# else
--
1.8.3.1
3:57 a.m.
New subject: [libvirt] [PATCH v1 06/32] util: netdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:21PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
...
@@ -1282,10 +1260,11 @@ virNetDevGetVirtualFunctions(const char *pfname,
{
int ret = -1;
size_t i;
- char *pf_sysfs_device_link = NULL;
- char *pci_sysfs_device_link = NULL;
- char *pciConfigAddr = NULL;
- char *pfPhysPortID = NULL;
+ VIR_AUTOFREE(char *) pf_sysfs_device_link = NULL;
+ VIR_AUTOFREE(char *) pci_sysfs_device_link = NULL;
+ VIR_AUTOFREE(char *) pciConfigAddr = NULL;
+ VIR_AUTOFREE(char *) pfPhysPortID = NULL;
+ VIR_AUTOFREE(char **) tempVfname = NULL;
First of all, this function should return the number of VFs on success or -1 on
error rather than needing the caller to pass an argument by reference to fill
the number of VFs, but that is a refactor for another day and I don't expect
you to spend time on that. Anyhow, tempVFname should be used instead of @vfname
at all spots and only at the end of the function block call VIR_STEAL_PTR.
*virt_fns = NULL;
*n_vfname = 0;
@@ -1333,13 +1312,9 @@ virNetDevGetVirtualFunctions(const char *pfname,
cleanup:
if (ret < 0) {
- VIR_FREE(*vfname);
+ VIR_STEAL_PTR(tempVfname, *vfname);
^This is not the intended usage of VIR_STEAL_PTR, the intended usage is to
steal the local pointer *into* the caller-provided one once we know we're going
to return success, not vice-versa, so ^this "if (ret < 0)" block should be
eventually dropped - you'd need another VIR_AUTOPTR for the virt_fns.
VIR_FREE(*virt_fns);
}
- VIR_FREE(pfPhysPortID);
- VIR_FREE(pf_sysfs_device_link);
- VIR_FREE(pci_sysfs_device_link);
- VIR_FREE(pciConfigAddr);
return ret;
}
...
@@ -1522,13 +1473,14 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
+ VIR_AUTOFREE(char *) pf_sysfs_path = NULL;
+ VIR_AUTOFREE(char *) vf_sysfs_path = NULL;
+ VIR_AUTOFREE(char *) tempPfname = NULL;
*pfname = NULL;
if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
- return ret;
+ return -1;
if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
goto cleanup;
@@ -1536,16 +1488,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char
**pfname,
if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
goto cleanup;
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
+ return virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
cleanup:
- if (ret < 0)
- VIR_FREE(*pfname);
+ VIR_STEAL_PTR(tempPfname, *pfname);
Same comment as above.
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
-
- return ret;
+ return -1;
}
...
#else /* !__linux__ */
cleanup:
nlmsg_free(nl_msg);
As I noted in previous responses, we can turn ^this into VIR_AUTOPTR too.
- VIR_FREE(resp);
return family_id;
}
@@ -3265,13 +3184,13 @@ virNetDevSwitchdevFeature(const char *ifname,
virBitmapPtr *out)
{
struct nl_msg *nl_msg = NULL;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
unsigned int recvbuflen;
struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
virPCIDevicePtr pci_device_ptr = NULL;
struct genlmsghdr* gmsgh = NULL;
const char *pci_name;
- char *pfname = NULL;
+ VIR_AUTOFREE(char *) pfname = NULL;
int is_vf = -1;
int ret = -1;
uint32_t family_id;
@@ -3333,8 +3252,6 @@ virNetDevSwitchdevFeature(const char *ifname,
cleanup:
nlmsg_free(nl_msg);
virPCIDeviceFree(pci_device_ptr);
- VIR_FREE(resp);
- VIR_FREE(pfname);
return ret;
}
# else
@@ -3375,7 +3292,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
int fd,
struct ifreq *ifr)
{
- struct ethtool_gfeatures *g_cmd;
+ VIR_AUTOFREE(struct ethtool_gfeatures *) g_cmd = NULL;
if (VIR_ALLOC_VAR(g_cmd,
struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
@@ -3385,7 +3302,6 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
g_cmd->size = GFEATURES_SIZE;
if (virNetDevGFeatureAvailable(fd, ifr, g_cmd))
ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL));
- VIR_FREE(g_cmd);
return 0;
}
# else
Otherwise, I don't see any other problems, since this will need another
version, the VIR_AUTOFREE declarations should move at the end of the "declare"
block within functions (like I said, it looks IMO better and separates regular
variables from the ones that can be freed automatically).
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 07/32] util: netdev: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 129 ++++++++++++++++++++++-----------------------------
1 file changed, 55 insertions(+), 74 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 7653f8b..c5871b4 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1855,16 +1855,15 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
const char *stateDir,
bool saveVlan)
{
- int ret = -1;
const char *pfDevName = NULL;
VIR_AUTOFREE(char *) pfDevOrig = NULL;
VIR_AUTOFREE(char *) vfDevOrig = NULL;
VIR_AUTOFREE(char *) filePath = NULL;
VIR_AUTOFREE(char *) fileStr = NULL;
+ VIR_AUTOPTR(virJSONValue) configJSON = NULL;
virMacAddr oldMAC;
char MACStr[VIR_MAC_STRING_BUFLEN];
int oldVlanTag = -1;
- virJSONValuePtr configJSON = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -1872,7 +1871,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
saveVlan = true;
@@ -1884,12 +1883,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
+ return -1;
}
if (pfDevName) {
@@ -1907,7 +1906,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* explicitly enable the PF in the host system network config.
*/
if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0)
- goto cleanup;
+ return -1;
if (!pfIsOnline) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1916,12 +1915,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
"change host network config to put the "
"PF online."),
vf, pfDevName);
- goto cleanup;
+ return -1;
}
}
if (!(configJSON = virJSONValueNewObject()))
- goto cleanup;
+ return -1;
/* if there is a PF, it's now in pfDevName, and linkdev is either
* the VF's name, or NULL (if the VF isn't bound to a net driver
@@ -1930,11 +1929,11 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
if (pfDevName && saveVlan) {
if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf)
< 0)
- goto cleanup;
+ return -1;
/* get admin MAC and vlan tag */
if (virNetDevGetVfConfig(pfDevName, vf, &oldMAC, &oldVlanTag) < 0)
- goto cleanup;
+ return -1;
if (virJSONValueObjectAppendString(configJSON,
VIR_NETDEV_KEYNAME_ADMIN_MAC,
@@ -1942,39 +1941,36 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
virJSONValueObjectAppendNumberInt(configJSON,
VIR_NETDEV_KEYNAME_VLAN_TAG,
oldVlanTag) < 0) {
- goto cleanup;
+ return -1;
}
} else {
if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0)
- goto cleanup;
+ return -1;
}
if (linkdev) {
if (virNetDevGetMAC(linkdev, &oldMAC) < 0)
- goto cleanup;
+ return -1;
/* for interfaces with no pfDevName (i.e. not a VF, this will
* be the only value in the file.
*/
if (virJSONValueObjectAppendString(configJSON, VIR_NETDEV_KEYNAME_MAC,
virMacAddrFormat(&oldMAC, MACStr)) <
0)
- goto cleanup;
+ return -1;
}
if (!(fileStr = virJSONValueToString(configJSON, true)))
- goto cleanup;
+ return -1;
if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
"for device = %s, vf = %d"), linkdev,
vf);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virJSONValueFree(configJSON);
- return ret;
+ return 0;
}
@@ -2012,7 +2008,10 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
VIR_AUTOFREE(char *) vfDevOrig = NULL;
VIR_AUTOFREE(char *) filePath = NULL;
VIR_AUTOFREE(char *) fileStr = NULL;
- virJSONValuePtr configJSON = NULL;
+ VIR_AUTOPTR(virJSONValue) configJSON = NULL;
+ VIR_AUTOPTR(virMacAddr) tempAdminMAC = NULL;
+ VIR_AUTOPTR(virMacAddr) tempMAC = NULL;
+ VIR_AUTOPTR(virNetDevVlan) tempVlan = NULL;
const char *MACStr = NULL;
const char *adminMACStr = NULL;
int vlanTag = -1;
@@ -2176,15 +2175,12 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
/* we won't need the file again */
ignore_value(unlink(filePath));
- ret = 0;
+ return 0;
cleanup:
- if (ret < 0) {
- VIR_FREE(*adminMAC);
- VIR_FREE(*MAC);
- VIR_FREE(*vlan);
- }
+ VIR_STEAL_PTR(tempAdminMAC, *adminMAC);
+ VIR_STEAL_PTR(tempMAC, *MAC);
+ VIR_STEAL_PTR(tempVlan, *vlan);
- virJSONValueFree(configJSON);
return ret;
}
@@ -2214,13 +2210,12 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
const virMacAddr *MAC,
bool setVlan)
{
- int ret = -1;
char MACStr[VIR_MAC_STRING_BUFLEN];
const char *pfDevName = NULL;
VIR_AUTOFREE(char *) pfDevOrig = NULL;
VIR_AUTOFREE(char *) vfDevOrig = NULL;
+ VIR_AUTOPTR(virPCIDevice) vfPCIDevice = NULL;
int vlanTag = -1;
- virPCIDevicePtr vfPCIDevice = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -2228,7 +2223,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
@@ -2239,12 +2234,12 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
+ return -1;
}
@@ -2256,14 +2251,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("admin MAC can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
if (vlan) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vlan can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
} else {
@@ -2272,14 +2267,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vlan trunking is not supported "
"by SR-IOV network devices"));
- goto cleanup;
+ return -1;
}
if (!setVlan) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("vlan tag set for interface %s but "
"caller requested it not be set"));
- goto cleanup;
+ return -1;
}
vlanTag = vlan->tag[0];
@@ -2297,7 +2292,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
_("VF %d of PF '%s' is not bound to a net driver,
"
"so its MAC address cannot be set to %s"),
vf, pfDevName, virMacAddrFormat(MAC, MACStr));
- goto cleanup;
+ return -1;
}
setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig);
@@ -2308,7 +2303,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* if pfDevOrig == NULL, this isn't a VF, so we've failed */
if (!pfDevOrig ||
(errno != EADDRNOTAVAIL && errno != EPERM))
- goto cleanup;
+ return -1;
/* Otherwise this is a VF, and virNetDevSetMAC failed with
* EADDRNOTAVAIL/EPERM, which could be due to the
@@ -2322,18 +2317,18 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
if (virNetDevSetVfConfig(pfDevName, vf,
MAC, vlanTag, &allowRetry) < 0) {
- goto cleanup;
+ return -1;
}
/* admin MAC is set, now we need to construct a virPCIDevice
* object so we can call virPCIDeviceRebind()
*/
if (!(vfPCIDevice = virNetDevGetPCIDevice(linkdev)))
- goto cleanup;
+ return -1;
/* Rebind the device. This should set the proper MAC address */
if (virPCIDeviceRebind(vfPCIDevice) < 0)
- goto cleanup;
+ return -1;
/* Wait until virNetDevGetIndex for the VF netdev returns success.
* This indicates that the device is ready to be used. If we don't
@@ -2385,20 +2380,17 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* with the "locally administered" bit set.
*/
if (!allowRetry)
- goto cleanup;
+ return -1;
allowRetry = false;
if (virNetDevSetVfConfig(pfDevName, vf,
&altZeroMAC, vlanTag, &allowRetry) < 0)
{
- goto cleanup;
+ return -1;
}
}
}
- ret = 0;
- cleanup:
- virPCIDeviceFree(vfPCIDevice);
- return ret;
+ return 0;
}
@@ -2873,30 +2865,31 @@ virNetDevRxFilterFree(virNetDevRxFilterPtr filter)
int virNetDevGetRxFilter(const char *ifname,
virNetDevRxFilterPtr *filter)
{
- int ret = -1;
bool receive = false;
- virNetDevRxFilterPtr fil = virNetDevRxFilterNew();
+ VIR_AUTOPTR(virNetDevRxFilter) fil = virNetDevRxFilterNew();
+
+ *filter = NULL;
if (!fil)
- goto cleanup;
+ return -1;
if (virNetDevGetMAC(ifname, &fil->mac))
- goto cleanup;
+ return -1;
if (virNetDevGetMulticastTable(ifname, fil))
- goto cleanup;
+ return -1;
if (virNetDevGetPromiscuous(ifname, &fil->promiscuous))
- goto cleanup;
+ return -1;
if (virNetDevGetRcvAllMulti(ifname, &receive))
- goto cleanup;
+ return -1;
if (receive) {
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_ALL;
} else {
if (virNetDevGetRcvMulti(ifname, &receive))
- goto cleanup;
+ return -1;
if (receive)
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_NORMAL;
@@ -2904,15 +2897,8 @@ int virNetDevGetRxFilter(const char *ifname,
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_NONE;
}
- ret = 0;
- cleanup:
- if (ret < 0) {
- virNetDevRxFilterFree(fil);
- fil = NULL;
- }
-
- *filter = fil;
- return ret;
+ VIR_STEAL_PTR(*filter, fil);
+ return 0;
}
#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
@@ -3185,12 +3171,12 @@ virNetDevSwitchdevFeature(const char *ifname,
{
struct nl_msg *nl_msg = NULL;
VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+ VIR_AUTOFREE(char *) pfname = NULL;
+ VIR_AUTOPTR(virPCIDevice) pci_device_ptr = NULL;
unsigned int recvbuflen;
struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
- virPCIDevicePtr pci_device_ptr = NULL;
struct genlmsghdr* gmsgh = NULL;
const char *pci_name;
- VIR_AUTOFREE(char *) pfname = NULL;
int is_vf = -1;
int ret = -1;
uint32_t family_id;
@@ -3251,7 +3237,6 @@ virNetDevSwitchdevFeature(const char *ifname,
cleanup:
nlmsg_free(nl_msg);
- virPCIDeviceFree(pci_device_ptr);
return ret;
}
# else
@@ -3507,8 +3492,7 @@ int virNetDevSetCoalesce(const char *ifname,
int
virNetDevRunEthernetScript(const char *ifname, const char *script)
{
- virCommandPtr cmd;
- int ret;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/* Not a bug! Previously we did accept script="" as a NO-OP. */
if (STREQ(script, ""))
@@ -3522,8 +3506,5 @@ virNetDevRunEthernetScript(const char *ifname, const char *script)
#endif
virCommandAddEnvPassCommon(cmd);
- ret = virCommandRun(cmd, NULL);
-
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
--
1.8.3.1
4:19 a.m.
New subject: [libvirt] [PATCH v1 07/32] util: netdev: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:22PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdev.c | 129 ++++++++++++++++++++++-----------------------------
1 file changed, 55 insertions(+), 74 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 7653f8b..c5871b4 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1855,16 +1855,15 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
const char *stateDir,
bool saveVlan)
{
- int ret = -1;
const char *pfDevName = NULL;
VIR_AUTOFREE(char *) pfDevOrig = NULL;
VIR_AUTOFREE(char *) vfDevOrig = NULL;
VIR_AUTOFREE(char *) filePath = NULL;
VIR_AUTOFREE(char *) fileStr = NULL;
+ VIR_AUTOPTR(virJSONValue) configJSON = NULL;
virMacAddr oldMAC;
char MACStr[VIR_MAC_STRING_BUFLEN];
int oldVlanTag = -1;
- virJSONValuePtr configJSON = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -1872,7 +1871,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
saveVlan = true;
@@ -1884,12 +1883,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
+ return -1;
}
if (pfDevName) {
@@ -1907,7 +1906,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* explicitly enable the PF in the host system network config.
*/
if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0)
- goto cleanup;
+ return -1;
if (!pfIsOnline) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1916,12 +1915,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
"change host network config to put the "
"PF online."),
vf, pfDevName);
- goto cleanup;
+ return -1;
}
}
if (!(configJSON = virJSONValueNewObject()))
- goto cleanup;
+ return -1;
/* if there is a PF, it's now in pfDevName, and linkdev is either
* the VF's name, or NULL (if the VF isn't bound to a net driver
@@ -1930,11 +1929,11 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
if (pfDevName && saveVlan) {
if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf)
< 0)
- goto cleanup;
+ return -1;
/* get admin MAC and vlan tag */
if (virNetDevGetVfConfig(pfDevName, vf, &oldMAC, &oldVlanTag) < 0)
- goto cleanup;
+ return -1;
if (virJSONValueObjectAppendString(configJSON,
VIR_NETDEV_KEYNAME_ADMIN_MAC,
@@ -1942,39 +1941,36 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
virJSONValueObjectAppendNumberInt(configJSON,
VIR_NETDEV_KEYNAME_VLAN_TAG,
oldVlanTag) < 0) {
- goto cleanup;
+ return -1;
}
} else {
if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0)
- goto cleanup;
+ return -1;
}
if (linkdev) {
if (virNetDevGetMAC(linkdev, &oldMAC) < 0)
- goto cleanup;
+ return -1;
/* for interfaces with no pfDevName (i.e. not a VF, this will
* be the only value in the file.
*/
if (virJSONValueObjectAppendString(configJSON, VIR_NETDEV_KEYNAME_MAC,
virMacAddrFormat(&oldMAC, MACStr)) <
0)
- goto cleanup;
+ return -1;
}
if (!(fileStr = virJSONValueToString(configJSON, true)))
- goto cleanup;
+ return -1;
if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
"for device = %s, vf = %d"), linkdev,
vf);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virJSONValueFree(configJSON);
- return ret;
+ return 0;
}
@@ -2012,7 +2008,10 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
VIR_AUTOFREE(char *) vfDevOrig = NULL;
VIR_AUTOFREE(char *) filePath = NULL;
VIR_AUTOFREE(char *) fileStr = NULL;
- virJSONValuePtr configJSON = NULL;
+ VIR_AUTOPTR(virJSONValue) configJSON = NULL;
+ VIR_AUTOPTR(virMacAddr) tempAdminMAC = NULL;
+ VIR_AUTOPTR(virMacAddr) tempMAC = NULL;
+ VIR_AUTOPTR(virNetDevVlan) tempVlan = NULL;
const char *MACStr = NULL;
const char *adminMACStr = NULL;
int vlanTag = -1;
@@ -2176,15 +2175,12 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
/* we won't need the file again */
ignore_value(unlink(filePath));
- ret = 0;
+ return 0;
cleanup:
- if (ret < 0) {
- VIR_FREE(*adminMAC);
- VIR_FREE(*MAC);
- VIR_FREE(*vlan);
- }
+ VIR_STEAL_PTR(tempAdminMAC, *adminMAC);
+ VIR_STEAL_PTR(tempMAC, *MAC);
+ VIR_STEAL_PTR(tempVlan, *vlan);
You'd want the opposite VIR_STEAL_PTR direction, so that the cleanup label
could be dropped completely.
- virJSONValueFree(configJSON);
return ret;
}
@@ -2214,13 +2210,12 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
const virMacAddr *MAC,
bool setVlan)
{
- int ret = -1;
char MACStr[VIR_MAC_STRING_BUFLEN];
const char *pfDevName = NULL;
VIR_AUTOFREE(char *) pfDevOrig = NULL;
VIR_AUTOFREE(char *) vfDevOrig = NULL;
+ VIR_AUTOPTR(virPCIDevice) vfPCIDevice = NULL;
int vlanTag = -1;
- virPCIDevicePtr vfPCIDevice = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -2228,7 +2223,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
@@ -2239,12 +2234,12 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
+ return -1;
}
@@ -2256,14 +2251,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("admin MAC can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
if (vlan) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vlan can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
} else {
@@ -2272,14 +2267,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vlan trunking is not supported "
"by SR-IOV network devices"));
- goto cleanup;
+ return -1;
}
if (!setVlan) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("vlan tag set for interface %s but "
"caller requested it not be set"));
- goto cleanup;
+ return -1;
}
vlanTag = vlan->tag[0];
@@ -2297,7 +2292,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
_("VF %d of PF '%s' is not bound to a net
driver, "
"so its MAC address cannot be set to %s"),
vf, pfDevName, virMacAddrFormat(MAC, MACStr));
- goto cleanup;
+ return -1;
}
setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig);
@@ -2308,7 +2303,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* if pfDevOrig == NULL, this isn't a VF, so we've failed */
if (!pfDevOrig ||
(errno != EADDRNOTAVAIL && errno != EPERM))
- goto cleanup;
+ return -1;
/* Otherwise this is a VF, and virNetDevSetMAC failed with
* EADDRNOTAVAIL/EPERM, which could be due to the
@@ -2322,18 +2317,18 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
if (virNetDevSetVfConfig(pfDevName, vf,
MAC, vlanTag, &allowRetry) < 0) {
- goto cleanup;
+ return -1;
}
/* admin MAC is set, now we need to construct a virPCIDevice
* object so we can call virPCIDeviceRebind()
*/
if (!(vfPCIDevice = virNetDevGetPCIDevice(linkdev)))
- goto cleanup;
+ return -1;
/* Rebind the device. This should set the proper MAC address */
if (virPCIDeviceRebind(vfPCIDevice) < 0)
- goto cleanup;
+ return -1;
/* Wait until virNetDevGetIndex for the VF netdev returns success.
* This indicates that the device is ready to be used. If we don't
@@ -2385,20 +2380,17 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* with the "locally administered" bit set.
*/
if (!allowRetry)
- goto cleanup;
+ return -1;
allowRetry = false;
if (virNetDevSetVfConfig(pfDevName, vf,
&altZeroMAC, vlanTag, &allowRetry) < 0)
{
- goto cleanup;
+ return -1;
}
}
}
- ret = 0;
- cleanup:
- virPCIDeviceFree(vfPCIDevice);
- return ret;
+ return 0;
}
@@ -2873,30 +2865,31 @@ virNetDevRxFilterFree(virNetDevRxFilterPtr filter)
int virNetDevGetRxFilter(const char *ifname,
virNetDevRxFilterPtr *filter)
{
- int ret = -1;
bool receive = false;
- virNetDevRxFilterPtr fil = virNetDevRxFilterNew();
+ VIR_AUTOPTR(virNetDevRxFilter) fil = virNetDevRxFilterNew();
+
+ *filter = NULL;
if (!fil)
- goto cleanup;
+ return -1;
if (virNetDevGetMAC(ifname, &fil->mac))
- goto cleanup;
+ return -1;
if (virNetDevGetMulticastTable(ifname, fil))
- goto cleanup;
+ return -1;
if (virNetDevGetPromiscuous(ifname, &fil->promiscuous))
- goto cleanup;
+ return -1;
if (virNetDevGetRcvAllMulti(ifname, &receive))
- goto cleanup;
+ return -1;
if (receive) {
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_ALL;
} else {
if (virNetDevGetRcvMulti(ifname, &receive))
- goto cleanup;
+ return -1;
if (receive)
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_NORMAL;
@@ -2904,15 +2897,8 @@ int virNetDevGetRxFilter(const char *ifname,
fil->multicast.mode = VIR_NETDEV_RX_FILTER_MODE_NONE;
}
- ret = 0;
- cleanup:
- if (ret < 0) {
- virNetDevRxFilterFree(fil);
- fil = NULL;
- }
-
- *filter = fil;
- return ret;
+ VIR_STEAL_PTR(*filter, fil);
Yep, this is the correct usage of VIR_STEAL_PTR.
+ return 0;
}
#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
@@ -3185,12 +3171,12 @@ virNetDevSwitchdevFeature(const char *ifname,
{
struct nl_msg *nl_msg = NULL;
VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+ VIR_AUTOFREE(char *) pfname = NULL;
^This code movement should be dropped and a we should follow a different scheme
separating the VIR_AUTO* variables. Otherwise looks fine.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 08/32] util: socketaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virSocketAddrPtr is declared using
VIR_AUTOPTR, the function virSocketAddrFree will be run
automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virsocketaddr.c | 7 ++++++-
src/util/virsocketaddr.h | 9 +++++++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 5c3bfad..6b52a3d 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -26,7 +26,6 @@
#include "virsocketaddr.h"
#include "virerror.h"
#include "virstring.h"
-#include "viralloc.h"
#include "virbuffer.h"
#include <netdb.h>
@@ -1253,3 +1252,9 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
ret = -2;
goto cleanup;
}
+
+void
+virSocketAddrFree(virSocketAddrPtr addr)
+{
+ VIR_FREE(addr);
+}
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h
index 3029338..66f5998 100644
--- a/src/util/virsocketaddr.h
+++ b/src/util/virsocketaddr.h
@@ -24,14 +24,15 @@
#ifndef __VIR_SOCKETADDR_H__
# define __VIR_SOCKETADDR_H__
-# include "internal.h"
-
# include <netinet/in.h>
# include <sys/socket.h>
# ifdef HAVE_SYS_UN_H
# include <sys/un.h>
# endif
+# include "internal.h"
+# include "viralloc.h"
+
/* On architectures which lack these limits, define them (ie. Cygwin).
* Note that the libvirt code should be robust enough to handle the
* case where actual value is longer than these limits (eg. by setting
@@ -162,4 +163,8 @@ int virSocketAddrPTRDomain(const virSocketAddr *addr,
char **ptr)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+void virSocketAddrFree(virSocketAddrPtr addr);
+
+VIR_DEFINE_AUTOPTR_FUNC(virSocketAddr, virSocketAddrFree)
+
#endif /* __VIR_SOCKETADDR_H__ */
--
1.8.3.1
6:25 a.m.
New subject: [libvirt] [PATCH v1 08/32] util: socketaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:23PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virSocketAddrPtr is declared using
VIR_AUTOPTR, the function virSocketAddrFree will be run
automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 09/32] util: socketaddr: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virsocketaddr.c | 70 ++++++++++++++++++++----------------------------
1 file changed, 29 insertions(+), 41 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 6b52a3d..eee725d 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -432,10 +432,10 @@ virSocketAddrFormatFull(const virSocketAddr *addr,
if (withService) {
if (virAsprintf(&addrstr, VIR_LOOPBACK_IPV4_ADDR"%s0",
separator ? separator : ":") < 0)
- goto error;
+ return NULL;
} else {
if (VIR_STRDUP(addrstr, VIR_LOOPBACK_IPV4_ADDR) < 0)
- goto error;
+ return NULL;
}
return addrstr;
}
@@ -452,33 +452,27 @@ virSocketAddrFormatFull(const virSocketAddr *addr,
}
if (withService) {
- char *ipv6_host = NULL;
+ VIR_AUTOFREE(char *) ipv6_host = NULL;
/* sasl_new_client demands the socket address to be in an odd format:
* a.b.c.d;port or e:f:g:h:i:j:k:l;port, so use square brackets for
* IPv6 only if no separator is passed to the function
*/
if (!separator && VIR_SOCKET_ADDR_FAMILY(addr) == AF_INET6) {
if (virAsprintf(&ipv6_host, "[%s]", host) < 0)
- goto error;
+ return NULL;
}
if (virAsprintf(&addrstr, "%s%s%s",
ipv6_host ? ipv6_host : host,
separator ? separator : ":", port) == -1) {
- VIR_FREE(ipv6_host);
- goto error;
+ return NULL;
}
-
- VIR_FREE(ipv6_host);
} else {
if (VIR_STRDUP(addrstr, host) < 0)
- goto error;
+ return NULL;
}
return addrstr;
-
- error:
- return NULL;
}
@@ -759,24 +753,26 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
int ret = 0;
size_t i;
virSocketAddr netmask;
- char *startStr = NULL, *endStr = NULL, *netStr = NULL;
+ VIR_AUTOFREE(char *) startStr = NULL;
+ VIR_AUTOFREE(char *) endStr = NULL;
+ VIR_AUTOFREE(char *) netStr = NULL;
if (start == NULL || end == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("NULL argument - %p %p"), start, end);
- goto error;
+ return -1;
}
startStr = virSocketAddrFormat(start);
endStr = virSocketAddrFormat(end);
if (!startStr || !endStr)
- goto error; /*error already reported */
+ return -1; /*error already reported */
if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("mismatch of address family in range %s - %s"),
startStr, endStr);
- goto error;
+ return -1;
}
if (network) {
@@ -784,14 +780,14 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
* network the range should be within
*/
if (!(netStr = virSocketAddrFormat(network)))
- goto error;
+ return -1;
if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("mismatch of address family in "
"range %s - %s for network %s"),
startStr, endStr, netStr);
- goto error;
+ return -1;
}
if (prefix < 0 ||
@@ -801,7 +797,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("bad prefix %d for network %s when "
" checking range %s - %s"),
prefix, netStr, startStr, endStr);
- goto error;
+ return -1;
}
/* both start and end of range need to be within network */
@@ -811,7 +807,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("range %s - %s is not entirely within "
"network %s/%d"),
startStr, endStr, netStr, prefix);
- goto error;
+ return -1;
}
if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) {
@@ -823,7 +819,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("failed to construct broadcast or network "
"address for network %s/%d"),
netStr, prefix);
- goto error;
+ return -1;
}
/* Don't allow the start of the range to be the network
@@ -838,7 +834,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("start of range %s - %s in network %s/%d "
"is the network address"),
startStr, endStr, netStr, prefix);
- goto error;
+ return -1;
}
if (virSocketAddrEqual(end, &broadcast)) {
@@ -846,7 +842,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("end of range %s - %s in network %s/%d "
"is the broadcast address"),
startStr, endStr, netStr, prefix);
- goto error;
+ return -1;
}
}
}
@@ -860,7 +856,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("failed to get IPv4 address "
"for start or end of range %s - %s"),
startStr, endStr);
- goto error;
+ return -1;
}
/* legacy check that everything except the last two bytes
@@ -868,10 +864,10 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
*/
for (i = 0; i < 2; i++) {
if (t1[i] != t2[i]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("range %s - %s is too large (> 65535)"),
- startStr, endStr);
- goto error;
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("range %s - %s is too large (> 65535)"),
+ startStr, endStr);
+ return -1;
}
}
ret = (t2[2] - t1[2]) * 256 + (t2[3] - t1[3]);
@@ -879,7 +875,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("range %s - %s is reversed "),
startStr, endStr);
- goto error;
+ return -1;
}
ret++;
} else if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET6)) {
@@ -891,7 +887,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("failed to get IPv6 address "
"for start or end of range %s - %s"),
startStr, endStr);
- goto error;
+ return -1;
}
/* legacy check that everything except the last two bytes are
@@ -902,7 +898,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("range %s - %s is too large (> 65535)"),
startStr, endStr);
- goto error;
+ return -1;
}
}
ret = t2[7] - t1[7];
@@ -910,7 +906,7 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("range %s - %s start larger than end"),
startStr, endStr);
- goto error;
+ return -1;
}
ret++;
} else {
@@ -918,18 +914,10 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end,
_("unsupported address family "
"for range %s - %s, must be ipv4 or ipv6"),
startStr, endStr);
- goto error;
+ return -1;
}
- cleanup:
- VIR_FREE(startStr);
- VIR_FREE(endStr);
- VIR_FREE(netStr);
return ret;
-
- error:
- ret = -1;
- goto cleanup;
}
--
1.8.3.1
7:49 a.m.
New subject: [libvirt] [PATCH v1 09/32] util: socketaddr: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:24PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 10/32] util: socketaddr: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virsocketaddr.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index eee725d..1b195cd 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1193,52 +1193,46 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
unsigned int prefix,
char **ptr)
{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
size_t i;
- int ret = -1;
+
+ if (VIR_ALLOC(buf) < 0)
+ return -1;
if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) {
virSocketAddrIPv4 ip;
if (prefix == 0 || prefix >= 32 || prefix % 8 != 0)
- goto unsupported;
+ return -2;
if (virSocketAddrGetIPv4Addr(addr, &ip) < 0)
- goto cleanup;
+ return -1;
for (i = prefix / 8; i > 0; i--)
- virBufferAsprintf(&buf, "%u.", ip[i - 1]);
+ virBufferAsprintf(buf, "%u.", ip[i - 1]);
- virBufferAddLit(&buf, VIR_SOCKET_ADDR_IPV4_ARPA);
+ virBufferAddLit(buf, VIR_SOCKET_ADDR_IPV4_ARPA);
} else if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6)) {
virSocketAddrIPv6Nibbles ip;
if (prefix == 0 || prefix >= 128 || prefix % 4 != 0)
- goto unsupported;
+ return -2;
if (virSocketAddrGetIPv6Nibbles(addr, &ip) < 0)
- goto cleanup;
+ return -1;
for (i = prefix / 4; i > 0; i--)
- virBufferAsprintf(&buf, "%x.", ip[i - 1]);
+ virBufferAsprintf(buf, "%x.", ip[i - 1]);
- virBufferAddLit(&buf, VIR_SOCKET_ADDR_IPV6_ARPA);
+ virBufferAddLit(buf, VIR_SOCKET_ADDR_IPV6_ARPA);
} else {
- goto unsupported;
+ return -2;
}
- if (!(*ptr = virBufferContentAndReset(&buf)))
- goto cleanup;
+ if (!(*ptr = virBufferContentAndReset(buf)))
+ return -1;
- ret = 0;
-
- cleanup:
- virBufferFreeAndReset(&buf);
- return ret;
-
- unsupported:
- ret = -2;
- goto cleanup;
+ return 0;
}
void
--
1.8.3.1
7:52 a.m.
New subject: [libvirt] [PATCH v1 10/32] util: socketaddr: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:25PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virsocketaddr.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index eee725d..1b195cd 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1193,52 +1193,46 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
unsigned int prefix,
char **ptr)
{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
I'll move this one line down.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
6:37 a.m.
New subject: [libvirt] [PATCH v1 10/32] util: socketaddr: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 02:52:12PM +0200, Erik Skultety wrote:
On Sat, Jul 28, 2018 at 11:31:25PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virsocketaddr.c | 38 ++++++++++++++++----------------------
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
> index eee725d..1b195cd 100644
> --- a/src/util/virsocketaddr.c
> +++ b/src/util/virsocketaddr.c
> @@ -1193,52 +1193,46 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
> unsigned int prefix,
> char **ptr)
> {
> - virBuffer buf = VIR_BUFFER_INITIALIZER;
> + VIR_AUTOPTR(virBuffer) buf = NULL;
I'll move this one line down.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
Actually, you probably noticed from patch 13, this has the obvious virBuffer
leak inside, so I retract my RB here.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 11/32] util: netdevip: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virNetDevIPAddrPtr and virNetDevIPRoutePtr
are declared using VIR_AUTOPTR, the functions virNetDevIPAddrFree
and virNetDevIPRouteFree, respectively, will be run
automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 7 ++++++-
src/util/virnetdevip.h | 4 ++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index bf98ed8..fdb0b74 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -27,7 +27,6 @@
#include "virnetlink.h"
#include "virfile.h"
#include "virerror.h"
-#include "viralloc.h"
#include "virlog.h"
#include "virstring.h"
#include "virutil.h"
@@ -1129,3 +1128,9 @@ virNetDevIPInfoAddToDev(const char *ifname,
cleanup:
return ret;
}
+
+void
+virNetDevIPAddrFree(virNetDevIPAddrPtr ip)
+{
+ VIR_FREE(ip);
+}
diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
index 6b509ea..5608c37 100644
--- a/src/util/virnetdevip.h
+++ b/src/util/virnetdevip.h
@@ -84,6 +84,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr)
int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
ATTRIBUTE_NONNULL(1);
bool virNetDevIPCheckIPv6Forwarding(void);
+void virNetDevIPAddrFree(virNetDevIPAddrPtr ip);
/* virNetDevIPRoute object */
void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
@@ -97,4 +98,7 @@ void virNetDevIPInfoClear(virNetDevIPInfoPtr ip);
int virNetDevIPInfoAddToDev(const char *ifname,
virNetDevIPInfo const *ipInfo);
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevIPAddr, virNetDevIPAddrFree)
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevIPRoute, virNetDevIPRouteFree)
+
#endif /* __VIR_NETDEVIP_H__ */
--
1.8.3.1
12:42 a.m.
New subject: [libvirt] [PATCH v1 11/32] util: netdevip: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:26PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virNetDevIPAddrPtr and virNetDevIPRoutePtr
are declared using VIR_AUTOPTR, the functions virNetDevIPAddrFree
and virNetDevIPRouteFree, respectively, will be run
automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 7 ++++++-
src/util/virnetdevip.h | 4 ++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index bf98ed8..fdb0b74 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -27,7 +27,6 @@
#include "virnetlink.h"
#include "virfile.h"
#include "virerror.h"
-#include "viralloc.h"
#include "virlog.h"
#include "virstring.h"
#include "virutil.h"
@@ -1129,3 +1128,9 @@ virNetDevIPInfoAddToDev(const char *ifname,
cleanup:
return ret;
}
+
+void
+virNetDevIPAddrFree(virNetDevIPAddrPtr ip)
+{
+ VIR_FREE(ip);
+}
diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
index 6b509ea..5608c37 100644
--- a/src/util/virnetdevip.h
+++ b/src/util/virnetdevip.h
@@ -84,6 +84,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr)
int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
ATTRIBUTE_NONNULL(1);
bool virNetDevIPCheckIPv6Forwarding(void);
+void virNetDevIPAddrFree(virNetDevIPAddrPtr ip);
These two hunks should be in a separate patch.
To the rest:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 12/32] util: netdevip: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 95 ++++++++++++++++++--------------------------------
1 file changed, 33 insertions(+), 62 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index fdb0b74..8f1081b 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -171,11 +171,11 @@ virNetDevIPAddrAdd(const char *ifname,
virSocketAddr *broadcast = NULL;
int ret = -1;
struct nl_msg *nlmsg = NULL;
- struct nlmsghdr *resp = NULL;
unsigned int recvbuflen;
- char *ipStr = NULL;
- char *peerStr = NULL;
- char *bcastStr = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+ VIR_AUTOFREE(char *) ipStr = NULL;
+ VIR_AUTOFREE(char *) peerStr = NULL;
+ VIR_AUTOFREE(char *) bcastStr = NULL;
ipStr = virSocketAddrFormat(addr);
if (peer && VIR_SOCKET_ADDR_VALID(peer))
@@ -225,11 +225,7 @@ virNetDevIPAddrAdd(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(ipStr);
- VIR_FREE(peerStr);
- VIR_FREE(bcastStr);
nlmsg_free(nlmsg);
- VIR_FREE(resp);
VIR_FREE(broadcast);
return ret;
}
@@ -252,7 +248,7 @@ virNetDevIPAddrDel(const char *ifname,
{
int ret = -1;
struct nl_msg *nlmsg = NULL;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
unsigned int recvbuflen;
if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_DELADDR, ifname,
@@ -273,7 +269,6 @@ virNetDevIPAddrDel(const char *ifname,
ret = 0;
cleanup:
nlmsg_free(nlmsg);
- VIR_FREE(resp);
return ret;
}
@@ -309,8 +304,8 @@ virNetDevIPRouteAdd(const char *ifname,
int errCode;
virSocketAddr defaultAddr;
virSocketAddrPtr actualAddr;
- char *toStr = NULL;
- char *viaStr = NULL;
+ VIR_AUTOFREE(char *) toStr = NULL;
+ VIR_AUTOFREE(char *) viaStr = NULL;
actualAddr = addr;
@@ -383,8 +378,6 @@ virNetDevIPRouteAdd(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(toStr);
- VIR_FREE(viaStr);
nlmsg_free(nlmsg);
return ret;
@@ -452,7 +445,6 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
{
struct nl_msg *nlmsg = NULL;
struct ifaddrmsg ifa;
- struct nlmsghdr *resp = NULL;
unsigned int recvbuflen;
int ret = -1;
bool dad = true;
@@ -475,6 +467,8 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
/* Periodically query netlink until DAD finishes on all known addresses. */
while (dad && time(NULL) < max_time) {
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+
if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0,
NETLINK_ROUTE, 0) < 0)
goto cleanup;
@@ -489,8 +483,6 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
dad = virNetDevIPParseDadStatus(resp, recvbuflen, addrs, count);
if (dad)
usleep(1000 * 10);
-
- VIR_FREE(resp);
}
/* Check timeout. */
if (dad) {
@@ -502,7 +494,6 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
}
cleanup:
- VIR_FREE(resp);
nlmsg_free(nlmsg);
return ret;
}
@@ -510,22 +501,18 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
static int
virNetDevIPGetAcceptRA(const char *ifname)
{
- char *path = NULL;
- char *buf = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
char *suffix;
int accept_ra = -1;
if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra",
ifname ? ifname : "all") < 0)
- goto cleanup;
+ return -1;
if ((virFileReadAll(path, 512, &buf) < 0) ||
(virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
- goto cleanup;
-
- cleanup:
- VIR_FREE(path);
- VIR_FREE(buf);
+ return accept_ra;
return accept_ra;
}
@@ -545,9 +532,8 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
struct rtmsg *rtmsg = NLMSG_DATA(resp);
int accept_ra = -1;
struct rtattr *rta;
- char *ifname = NULL;
+ VIR_AUTOFREE(char *) ifname = NULL;
struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
- int ret = 0;
int len = RTM_PAYLOAD(resp);
int oif = -1;
size_t i;
@@ -555,7 +541,7 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
/* Ignore messages other than route ones */
if (resp->nlmsg_type != RTM_NEWROUTE)
- return ret;
+ return 0;
/* Extract a device ID attribute */
VIR_WARNINGS_NO_CAST_ALIGN
@@ -566,21 +552,20 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
/* Should never happen: netlink message would be broken */
if (ifname) {
- char *ifname2 = virNetDevGetName(oif);
+ VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif);
VIR_WARN("Single route has unexpected 2nd interface "
"- '%s' and '%s'", ifname, ifname2);
- VIR_FREE(ifname2);
break;
}
if (!(ifname = virNetDevGetName(oif)))
- goto error;
+ return -1;
}
}
/* No need to do anything else for non RA routes */
if (rtmsg->rtm_protocol != RTPROT_RA)
- goto cleanup;
+ return 0;
data->hasRARoutes = true;
@@ -595,15 +580,9 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
}
if (accept_ra != 2 && !hasDevice &&
VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
- goto error;
+ return -1;
- cleanup:
- VIR_FREE(ifname);
- return ret;
-
- error:
- ret = -1;
- goto cleanup;
+ return 0;
}
bool
@@ -687,7 +666,9 @@ virNetDevIPAddrAdd(const char *ifname,
unsigned int prefix)
{
virCommandPtr cmd = NULL;
- char *addrstr = NULL, *bcaststr = NULL, *peerstr = NULL;
+ VIR_AUTOFREE(char *) addrstr = NULL;
+ VIR_AUTOFREE(char *) bcaststr = NULL;
+ VIR_AUTOFREE(char *) peerstr = NULL;
virSocketAddr broadcast;
int ret = -1;
@@ -733,9 +714,6 @@ virNetDevIPAddrAdd(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(addrstr);
- VIR_FREE(bcaststr);
- VIR_FREE(peerstr);
virCommandFree(cmd);
return ret;
}
@@ -747,7 +725,7 @@ virNetDevIPAddrDel(const char *ifname,
unsigned int prefix)
{
virCommandPtr cmd = NULL;
- char *addrstr;
+ VIR_AUTOFREE(char *) addrstr = NULL;
int ret = -1;
if (!(addrstr = virSocketAddrFormat(addr)))
@@ -773,7 +751,6 @@ virNetDevIPAddrDel(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(addrstr);
virCommandFree(cmd);
return ret;
}
@@ -787,7 +764,8 @@ virNetDevIPRouteAdd(const char *ifname,
unsigned int metric)
{
virCommandPtr cmd = NULL;
- char *addrstr = NULL, *gatewaystr = NULL;
+ VIR_AUTOFREE(char *) addrstr = NULL;
+ VIR_AUTOFREE(char *) gatewaystr = NULL;
int ret = -1;
if (!(addrstr = virSocketAddrFormat(addr)))
@@ -806,8 +784,6 @@ virNetDevIPRouteAdd(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(addrstr);
- VIR_FREE(gatewaystr);
virCommandFree(cmd);
return ret;
}
@@ -1083,7 +1059,6 @@ int
virNetDevIPInfoAddToDev(const char *ifname,
virNetDevIPInfo const *ipInfo)
{
- int ret = -1;
size_t i;
int prefix;
@@ -1093,16 +1068,15 @@ virNetDevIPInfoAddToDev(const char *ifname,
if ((prefix = virSocketAddrGetIPPrefix(&ip->address,
NULL, ip->prefix)) < 0) {
- char *ipStr = virSocketAddrFormat(&ip->address);
+ VIR_AUTOFREE(char *) ipStr = virSocketAddrFormat(&ip->address);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to determine prefix for IP address
'%s'"),
NULLSTR(ipStr));
- VIR_FREE(ipStr);
- goto cleanup;
+ return -1;
}
if (virNetDevIPAddrAdd(ifname, &ip->address, &ip->peer, prefix)
< 0)
- goto cleanup;
+ return -1;
}
/* add all routes */
@@ -1110,23 +1084,20 @@ virNetDevIPInfoAddToDev(const char *ifname,
virNetDevIPRoutePtr route = ipInfo->routes[i];
if ((prefix = virNetDevIPRouteGetPrefix(route)) < 0) {
- char *ipStr = virSocketAddrFormat(&route->address);
+ VIR_AUTOFREE(char *) ipStr = virSocketAddrFormat(&route->address);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to determine prefix for route with destination
'%s'"),
NULLSTR(ipStr));
- VIR_FREE(ipStr);
- goto cleanup;
+ return -1;
}
if (virNetDevIPRouteAdd(ifname, &route->address, prefix,
&route->gateway,
virNetDevIPRouteGetMetric(route)) < 0)
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
void
--
1.8.3.1
7:54 a.m.
New subject: [libvirt] [PATCH v1 12/32] util: netdevip: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:27PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 95 ++++++++++++++++++--------------------------------
1 file changed, 33 insertions(+), 62 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index fdb0b74..8f1081b 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -171,11 +171,11 @@ virNetDevIPAddrAdd(const char *ifname,
virSocketAddr *broadcast = NULL;
int ret = -1;
struct nl_msg *nlmsg = NULL;
As I pointed out earlier, ^this could be converted to VIR_AUTOPTR. Otherwise
it's fine.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 55 +++++++++++++++++++++-----------------------------
1 file changed, 23 insertions(+), 32 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 8f1081b..ca206e2 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
}
if (!valid) {
- virBuffer buf = VIR_BUFFER_INITIALIZER;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
+
+ if (VIR_ALLOC(buf) < 0)
+ goto cleanup;
+
for (i = 0; i < data.ndevices; i++) {
- virBufferAdd(&buf, data.devices[i], -1);
+ virBufferAdd(buf, data.devices[i], -1);
if (i < data.ndevices - 1)
- virBufferAddLit(&buf, ", ");
+ virBufferAddLit(buf, ", ");
}
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Check the host setup: enabling IPv6 forwarding with
"
"RA routes without accept_ra set to 2 is likely to cause
"
"routes loss. Interfaces to look at: %s"),
- virBufferCurrentContent(&buf));
- virBufferFreeAndReset(&buf);
+ virBufferCurrentContent(buf));
}
cleanup:
@@ -665,24 +668,23 @@ virNetDevIPAddrAdd(const char *ifname,
virSocketAddr *peer,
unsigned int prefix)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) addrstr = NULL;
VIR_AUTOFREE(char *) bcaststr = NULL;
VIR_AUTOFREE(char *) peerstr = NULL;
virSocketAddr broadcast;
- int ret = -1;
if (!(addrstr = virSocketAddrFormat(addr)))
- goto cleanup;
+ return -1;
if (peer && VIR_SOCKET_ADDR_VALID(peer) && !(peerstr =
virSocketAddrFormat(peer)))
- goto cleanup;
+ return -1;
/* format up a broadcast address if this is IPv4 */
if (!peerstr && ((VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) &&
((virSocketAddrBroadcastByPrefix(addr, prefix, &broadcast) < 0) ||
!(bcaststr = virSocketAddrFormat(&broadcast))))) {
- goto cleanup;
+ return -1;
}
# ifdef IFCONFIG_PATH
@@ -710,12 +712,9 @@ virNetDevIPAddrAdd(const char *ifname,
# endif
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -724,12 +723,11 @@ virNetDevIPAddrDel(const char *ifname,
virSocketAddr *addr,
unsigned int prefix)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) addrstr = NULL;
- int ret = -1;
if (!(addrstr = virSocketAddrFormat(addr)))
- goto cleanup;
+ return -1;
# ifdef IFCONFIG_PATH
cmd = virCommandNew(IFCONFIG_PATH);
virCommandAddArg(cmd, ifname);
@@ -747,12 +745,9 @@ virNetDevIPAddrDel(const char *ifname,
# endif
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -763,15 +758,14 @@ virNetDevIPRouteAdd(const char *ifname,
virSocketAddrPtr gateway,
unsigned int metric)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) addrstr = NULL;
VIR_AUTOFREE(char *) gatewaystr = NULL;
- int ret = -1;
if (!(addrstr = virSocketAddrFormat(addr)))
- goto cleanup;
+ return -1;
if (!(gatewaystr = virSocketAddrFormat(gateway)))
- goto cleanup;
+ return -1;
cmd = virCommandNew(IP_PATH);
virCommandAddArgList(cmd, "route", "add", NULL);
virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
@@ -780,12 +774,9 @@ virNetDevIPRouteAdd(const char *ifname,
virCommandAddArgFormat(cmd, "%u", metric);
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
--
1.8.3.1
8:02 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevip.c | 55 +++++++++++++++++++++-----------------------------
1 file changed, 23 insertions(+), 32 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 8f1081b..ca206e2 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
}
if (!valid) {
- virBuffer buf = VIR_BUFFER_INITIALIZER;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
+
+ if (VIR_ALLOC(buf) < 0)
+ goto cleanup;
Hmm, this will actually leak memory because @buf is never going to be freed,
worse, we'll assign NULL to it.
Erik
8:08 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:32, Erik Skultety <eskultet(a)redhat.com> wrote:
On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virnetdevip.c | 55 +++++++++++++++++++++-----------------------------
> 1 file changed, 23 insertions(+), 32 deletions(-)
>
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 8f1081b..ca206e2 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> }
>
> if (!valid) {
> - virBuffer buf = VIR_BUFFER_INITIALIZER;
> + VIR_AUTOPTR(virBuffer) buf = NULL;
> +
> + if (VIR_ALLOC(buf) < 0)
> + goto cleanup;
Hmm, this will actually leak memory because @buf is never going to be freed,
worse, we'll assign NULL to it.
But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called
when it exits the scope, right?
If I were using virBufferContentAndReset, then it might be the case.
8:11 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote:
On Fri, 3 Aug 2018 at 18:32, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > majority of the calls to *Free functions can be dropped, which
> > in turn leads to getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> > src/util/virnetdevip.c | 55
+++++++++++++++++++++-----------------------------
> > 1 file changed, 23 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > index 8f1081b..ca206e2 100644
> > --- a/src/util/virnetdevip.c
> > +++ b/src/util/virnetdevip.c
> > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> > }
> >
> > if (!valid) {
> > - virBuffer buf = VIR_BUFFER_INITIALIZER;
> > + VIR_AUTOPTR(virBuffer) buf = NULL;
> > +
> > + if (VIR_ALLOC(buf) < 0)
> > + goto cleanup;
>
> Hmm, this will actually leak memory because @buf is never going to be freed,
> worse, we'll assign NULL to it.
But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called
when it exits the scope, right?
If I were using virBufferContentAndReset, then it might be the case.
How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps
@buf.
Erik
8:16 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:41, Erik Skultety <eskultet(a)redhat.com> wrote:
On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote:
> On Fri, 3 Aug 2018 at 18:32, Erik Skultety <eskultet(a)redhat.com> wrote:
> >
> > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > majority of the calls to *Free functions can be dropped, which
> > > in turn leads to getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > > ---
> > > src/util/virnetdevip.c | 55
+++++++++++++++++++++-----------------------------
> > > 1 file changed, 23 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > > index 8f1081b..ca206e2 100644
> > > --- a/src/util/virnetdevip.c
> > > +++ b/src/util/virnetdevip.c
> > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> > > }
> > >
> > > if (!valid) {
> > > - virBuffer buf = VIR_BUFFER_INITIALIZER;
> > > + VIR_AUTOPTR(virBuffer) buf = NULL;
> > > +
> > > + if (VIR_ALLOC(buf) < 0)
> > > + goto cleanup;
> >
> > Hmm, this will actually leak memory because @buf is never going to be freed,
> > worse, we'll assign NULL to it.
>
> But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called
> when it exits the scope, right?
> If I were using virBufferContentAndReset, then it might be the case.
How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps
@buf.
Got it. I actually made a lot of such changes, have to revert them all.
8:28 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 06:46:28PM +0530, Sukrit Bhatnagar wrote:
On Fri, 3 Aug 2018 at 18:41, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote:
> > On Fri, 3 Aug 2018 at 18:32, Erik Skultety <eskultet(a)redhat.com> wrote:
> > >
> > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> > > > By making use of GNU C's cleanup attribute handled by the
> > > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > > majority of the calls to *Free functions can be dropped, which
> > > > in turn leads to getting rid of most of our cleanup sections.
> > > >
> > > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > > > ---
> > > > src/util/virnetdevip.c | 55
+++++++++++++++++++++-----------------------------
> > > > 1 file changed, 23 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > > > index 8f1081b..ca206e2 100644
> > > > --- a/src/util/virnetdevip.c
> > > > +++ b/src/util/virnetdevip.c
> > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> > > > }
> > > >
> > > > if (!valid) {
> > > > - virBuffer buf = VIR_BUFFER_INITIALIZER;
> > > > + VIR_AUTOPTR(virBuffer) buf = NULL;
> > > > +
> > > > + if (VIR_ALLOC(buf) < 0)
> > > > + goto cleanup;
> > >
> > > Hmm, this will actually leak memory because @buf is never going to be
freed,
> > > worse, we'll assign NULL to it.
> >
> > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called
> > when it exits the scope, right?
> > If I were using virBufferContentAndReset, then it might be the case.
>
> How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps
> @buf.
Got it. I actually made a lot of such changes, have to revert them all.
Luckily I don't see any of that merged yet, so at least we don't have to hunt
it down in the release.
Erik
8:29 a.m.
New subject: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:58, Erik Skultety <eskultet(a)redhat.com> wrote:
On Fri, Aug 03, 2018 at 06:46:28PM +0530, Sukrit Bhatnagar wrote:
> On Fri, 3 Aug 2018 at 18:41, Erik Skultety <eskultet(a)redhat.com> wrote:
> >
> > On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote:
> > > On Fri, 3 Aug 2018 at 18:32, Erik Skultety <eskultet(a)redhat.com>
wrote:
> > > >
> > > > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> > > > > By making use of GNU C's cleanup attribute handled by the
> > > > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > > > majority of the calls to *Free functions can be dropped, which
> > > > > in turn leads to getting rid of most of our cleanup sections.
> > > > >
> > > > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > > > > ---
> > > > > src/util/virnetdevip.c | 55
+++++++++++++++++++++-----------------------------
> > > > > 1 file changed, 23 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > > > > index 8f1081b..ca206e2 100644
> > > > > --- a/src/util/virnetdevip.c
> > > > > +++ b/src/util/virnetdevip.c
> > > > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> > > > > }
> > > > >
> > > > > if (!valid) {
> > > > > - virBuffer buf = VIR_BUFFER_INITIALIZER;
> > > > > + VIR_AUTOPTR(virBuffer) buf = NULL;
> > > > > +
> > > > > + if (VIR_ALLOC(buf) < 0)
> > > > > + goto cleanup;
> > > >
> > > > Hmm, this will actually leak memory because @buf is never going to be
freed,
> > > > worse, we'll assign NULL to it.
> > >
> > > But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be
called
> > > when it exits the scope, right?
> > > If I were using virBufferContentAndReset, then it might be the case.
> >
> > How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps
> > @buf.
>
> Got it. I actually made a lot of such changes, have to revert them all.
Luckily I don't see any of that merged yet, so at least we don't have to hunt
it down in the release.
Yes. Most of the changes are in the patches I haven't posted here yet.
1:01 p.m.
New subject: [libvirt] [PATCH v1 14/32] util: netdevmacvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virNetlinkCallbackDataPtr is declared using
VIR_AUTOPTR, the function virNetlinkCallbackDataFree will be run
automatically on it when it goes out of scope.
This commit also adds an intermediate typedef for virNetlinkCallbackData
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevmacvlan.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index fb41bf9..91c6244 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -576,7 +576,7 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
};
/* Struct to hold the state and configuration of a 802.1qbg port */
-struct virNetlinkCallbackData {
+struct _virNetlinkCallbackData {
char *cr_ifname;
virNetDevVPortProfilePtr virtPortProfile;
virMacAddr macaddress;
@@ -587,7 +587,8 @@ struct virNetlinkCallbackData {
unsigned int linkState;
};
-typedef struct virNetlinkCallbackData *virNetlinkCallbackDataPtr;
+typedef struct _virNetlinkCallbackData virNetlinkCallbackData;
+typedef virNetlinkCallbackData *virNetlinkCallbackDataPtr;
# define INSTANCE_STRLEN 36
@@ -870,6 +871,8 @@ virNetlinkCallbackDataFree(virNetlinkCallbackDataPtr calld)
VIR_FREE(calld);
}
+VIR_DEFINE_AUTOPTR_FUNC(virNetlinkCallbackData, virNetlinkCallbackDataFree)
+
/**
* virNetDevMacVLanVPortProfileDestroyCallback:
*
--
1.8.3.1
8:22 a.m.
New subject: [libvirt] [PATCH v1 14/32] util: netdevmacvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:29PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virNetlinkCallbackDataPtr is declared using
VIR_AUTOPTR, the function virNetlinkCallbackDataFree will be run
automatically on it when it goes out of scope.
This commit also adds an intermediate typedef for virNetlinkCallbackData
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevmacvlan.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index fb41bf9..91c6244 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -576,7 +576,7 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
};
/* Struct to hold the state and configuration of a 802.1qbg port */
-struct virNetlinkCallbackData {
+struct _virNetlinkCallbackData {
char *cr_ifname;
virNetDevVPortProfilePtr virtPortProfile;
virMacAddr macaddress;
@@ -587,7 +587,8 @@ struct virNetlinkCallbackData {
unsigned int linkState;
};
-typedef struct virNetlinkCallbackData *virNetlinkCallbackDataPtr;
+typedef struct _virNetlinkCallbackData virNetlinkCallbackData;
+typedef virNetlinkCallbackData *virNetlinkCallbackDataPtr;
# define INSTANCE_STRLEN 36
@@ -870,6 +871,8 @@ virNetlinkCallbackDataFree(virNetlinkCallbackDataPtr calld)
VIR_FREE(calld);
}
+VIR_DEFINE_AUTOPTR_FUNC(virNetlinkCallbackData, virNetlinkCallbackDataFree)
I believe we can add this once we have a better use case, right now, we don't,
so this patch can be dropped.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 15/32] util: netdevmacvlan: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevmacvlan.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 91c6244..a2ed65c 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -308,7 +308,7 @@ virNetDevMacVLanCreate(const char *ifname,
int *retry)
{
int rc = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
int ifindex;
@@ -403,7 +403,6 @@ virNetDevMacVLanCreate(const char *ifname,
rc = 0;
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -452,7 +451,7 @@ virNetDevMacVLanTapOpen(const char *ifname,
{
int ret = -1;
int ifindex;
- char *tapname = NULL;
+ VIR_AUTOFREE(char *) tapname = NULL;
size_t i = 0;
if (virNetDevGetIndex(ifname, &ifindex) < 0)
@@ -487,7 +486,6 @@ virNetDevMacVLanTapOpen(const char *ifname,
while (i--)
VIR_FORCE_CLOSE(tapfd[i]);
}
- VIR_FREE(tapname);
return ret;
}
--
1.8.3.1
8:12 a.m.
New subject: [libvirt] [PATCH v1 15/32] util: netdevmacvlan: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:30PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevmacvlan.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index a2ed65c..d01e5ef 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
virNetDevVPortProfilePtr virtPortProfile,
virNetDevVPortProfileOp vmOp)
{
- virNetlinkCallbackDataPtr calld = NULL;
+ VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
+ virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
if (VIR_ALLOC(calld) < 0)
- goto error;
+ return -1;
if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
- goto error;
+ return -1;
if (VIR_ALLOC(calld->virtPortProfile) < 0)
- goto error;
+ return -1;
memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
virMacAddrSet(&calld->macaddress, macaddress);
if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
- goto error;
+ return -1;
memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
calld->vmOp = vmOp;
@@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
virNetDevMacVLanVPortProfileDestroyCallback,
calld, macaddress, NETLINK_ROUTE) < 0)
- goto error;
+ return -1;
}
+ VIR_STEAL_PTR(temp, calld);
+
return 0;
-
- error:
- virNetlinkCallbackDataFree(calld);
- return -1;
}
@@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
}
if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
- virMacAddrPtr MAC = NULL;
- virMacAddrPtr adminMAC = NULL;
- virNetDevVlanPtr vlan = NULL;
+ VIR_AUTOPTR(virMacAddr) MAC = NULL;
+ VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
+ VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
&adminMAC, &vlan, &MAC) == 0) &&
@@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
ignore_value(virNetDevSetNetConfig(linkdev, -1,
adminMAC, vlan, MAC, !!vlan));
- VIR_FREE(MAC);
- VIR_FREE(adminMAC);
- virNetDevVlanFree(vlan);
}
}
--
1.8.3.1
8:19 a.m.
New subject: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:31PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevmacvlan.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index a2ed65c..d01e5ef 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
virNetDevVPortProfilePtr virtPortProfile,
virNetDevVPortProfileOp vmOp)
{
- virNetlinkCallbackDataPtr calld = NULL;
+ VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
+ virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
if (VIR_ALLOC(calld) < 0)
- goto error;
+ return -1;
if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
- goto error;
+ return -1;
if (VIR_ALLOC(calld->virtPortProfile) < 0)
- goto error;
+ return -1;
memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
virMacAddrSet(&calld->macaddress, macaddress);
if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
- goto error;
+ return -1;
memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
calld->vmOp = vmOp;
@@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
virNetDevMacVLanVPortProfileDestroyCallback,
calld, macaddress, NETLINK_ROUTE) < 0)
- goto error;
+ return -1;
}
+ VIR_STEAL_PTR(temp, calld);
+
return 0;
-
- error:
- virNetlinkCallbackDataFree(calld);
- return -1;
}
^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
here and should be left as is.
@@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
}
if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
- virMacAddrPtr MAC = NULL;
- virMacAddrPtr adminMAC = NULL;
- virNetDevVlanPtr vlan = NULL;
+ VIR_AUTOPTR(virMacAddr) MAC = NULL;
+ VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
+ VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
&adminMAC, &vlan, &MAC) == 0)
&&
@@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
ignore_value(virNetDevSetNetConfig(linkdev, -1,
adminMAC, vlan, MAC, !!vlan));
- VIR_FREE(MAC);
- VIR_FREE(adminMAC);
- virNetDevVlanFree(vlan);
}
To ^this hunk:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:26 p.m.
New subject: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:49, Erik Skultety <eskultet(a)redhat.com> wrote:
On Sat, Jul 28, 2018 at 11:31:31PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
> src/util/virnetdevmacvlan.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index a2ed65c..d01e5ef 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
*ifname,
> virNetDevVPortProfilePtr
virtPortProfile,
> virNetDevVPortProfileOp vmOp)
> {
> - virNetlinkCallbackDataPtr calld = NULL;
> + VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> + virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
>
> if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE))
{
> if (VIR_ALLOC(calld) < 0)
> - goto error;
> + return -1;
> if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> - goto error;
> + return -1;
> if (VIR_ALLOC(calld->virtPortProfile) < 0)
> - goto error;
> + return -1;
> memcpy(calld->virtPortProfile, virtPortProfile,
sizeof(*virtPortProfile));
> virMacAddrSet(&calld->macaddress, macaddress);
> if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> - goto error;
> + return -1;
> memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
>
> calld->vmOp = vmOp;
> @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
*ifname,
> if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> virNetDevMacVLanVPortProfileDestroyCallback,
> calld, macaddress, NETLINK_ROUTE) < 0)
> - goto error;
> + return -1;
> }
>
> + VIR_STEAL_PTR(temp, calld);
> +
> return 0;
> -
> - error:
> - virNetlinkCallbackDataFree(calld);
> - return -1;
> }
^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
here and should be left as is.
But by doing this, are getting rid of goto jumps and error section.
That was one of the goals, right?
>
>
> @@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
*ifname,
> }
>
> if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> - virMacAddrPtr MAC = NULL;
> - virMacAddrPtr adminMAC = NULL;
> - virNetDevVlanPtr vlan = NULL;
> + VIR_AUTOPTR(virMacAddr) MAC = NULL;
> + VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
> + VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
>
> if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
> &adminMAC, &vlan, &MAC) == 0)
&&
> @@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
*ifname,
>
> ignore_value(virNetDevSetNetConfig(linkdev, -1,
> adminMAC, vlan, MAC, !!vlan));
> - VIR_FREE(MAC);
> - VIR_FREE(adminMAC);
> - virNetDevVlanFree(vlan);
> }
To ^this hunk:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
> }
>
> --
> 1.8.3.1
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:27 a.m.
New subject: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 10:56:40PM +0530, Sukrit Bhatnagar wrote:
On Fri, 3 Aug 2018 at 18:49, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:31PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > majority of the calls to *Free functions can be dropped, which
> > in turn leads to getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> > src/util/virnetdevmacvlan.c | 28 ++++++++++++----------------
> > 1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> > index a2ed65c..d01e5ef 100644
> > --- a/src/util/virnetdevmacvlan.c
> > +++ b/src/util/virnetdevmacvlan.c
> > @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
*ifname,
> > virNetDevVPortProfilePtr
virtPortProfile,
> > virNetDevVPortProfileOp vmOp)
> > {
> > - virNetlinkCallbackDataPtr calld = NULL;
> > + VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> > + virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
> >
> > if (virtPortProfile &&
virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
> > if (VIR_ALLOC(calld) < 0)
> > - goto error;
> > + return -1;
> > if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> > - goto error;
> > + return -1;
> > if (VIR_ALLOC(calld->virtPortProfile) < 0)
> > - goto error;
> > + return -1;
> > memcpy(calld->virtPortProfile, virtPortProfile,
sizeof(*virtPortProfile));
> > virMacAddrSet(&calld->macaddress, macaddress);
> > if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> > - goto error;
> > + return -1;
> > memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
> >
> > calld->vmOp = vmOp;
> > @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
*ifname,
> > if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> >
virNetDevMacVLanVPortProfileDestroyCallback,
> > calld, macaddress, NETLINK_ROUTE) <
0)
> > - goto error;
> > + return -1;
> > }
> >
> > + VIR_STEAL_PTR(temp, calld);
> > +
> > return 0;
> > -
> > - error:
> > - virNetlinkCallbackDataFree(calld);
> > - return -1;
> > }
>
> ^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
> here and should be left as is.
But by doing this, are getting rid of goto jumps and error section.
That was one of the goals, right?
Yes, it was the goal, but we can't do it blindly everywhere just for the sake
of replacement, for this specific case, we didn't really gain anything, because
we traded one error label containing a single *Free call for one extra
virNetlinkCallbackDataPtr variable just to be able to do a VIR_STEAL_PTR
(which again, should be mainly used for caller-provided pointers),
virNetlinkCallbackDataPtr typedefs and VIR_DEFINE_AUTOPTR_FUNC for a single use
case.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 17/32] util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevopenvswitch.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index d1c5cf4..a9c5e2a 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -149,10 +149,10 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
char macaddrstr[VIR_MAC_STRING_BUFLEN];
char ifuuidstr[VIR_UUID_STRING_BUFLEN];
char vmuuidstr[VIR_UUID_STRING_BUFLEN];
- char *attachedmac_ex_id = NULL;
- char *ifaceid_ex_id = NULL;
- char *profile_ex_id = NULL;
- char *vmid_ex_id = NULL;
+ VIR_AUTOFREE(char *) attachedmac_ex_id = NULL;
+ VIR_AUTOFREE(char *) ifaceid_ex_id = NULL;
+ VIR_AUTOFREE(char *) profile_ex_id = NULL;
+ VIR_AUTOFREE(char *) vmid_ex_id = NULL;
virMacAddrFormat(macaddr, macaddrstr);
virUUIDFormat(ovsport->interfaceID, ifuuidstr);
@@ -209,10 +209,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
ret = 0;
cleanup:
- VIR_FREE(attachedmac_ex_id);
- VIR_FREE(ifaceid_ex_id);
- VIR_FREE(vmid_ex_id);
- VIR_FREE(profile_ex_id);
virCommandFree(cmd);
return ret;
}
@@ -339,7 +335,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats)
{
virCommandPtr cmd = NULL;
- char *output;
+ VIR_AUTOFREE(char *) output = NULL;
char *tmp;
bool gotStats = false;
int ret = -1;
@@ -399,7 +395,6 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
ret = 0;
cleanup:
- VIR_FREE(output);
virCommandFree(cmd);
return ret;
}
@@ -424,7 +419,6 @@ int
virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
{
virCommandPtr cmd = NULL;
- int ret = -1;
int exitstatus;
*master = NULL;
@@ -438,7 +432,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to run command to get OVS master for "
"interface %s"), ifname);
- goto cleanup;
+ return -1;
}
/* non-0 exit code just means that the interface has no master in OVS */
@@ -454,9 +448,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master :
"(none)");
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
@@ -476,12 +468,12 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
char **ifname)
{
virCommandPtr cmd = NULL;
+ VIR_AUTOFREE(char *) ovs_timeout = NULL;
char *tmpIfname = NULL;
char **tokens = NULL;
size_t ntokens = 0;
int status;
int ret = -1;
- char *ovs_timeout = NULL;
/* Openvswitch vhostuser path are hardcoded to
* /<runstatedir>/openvswitch/<ifname>
@@ -513,7 +505,6 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
cleanup:
virStringListFreeCount(tokens, ntokens);
virCommandFree(cmd);
- VIR_FREE(ovs_timeout);
return ret;
}
--
1.8.3.1
8:32 a.m.
New subject: [libvirt] [PATCH v1 17/32] util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:32PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
...
@@ -424,7 +419,6 @@ int
virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
{
virCommandPtr cmd = NULL;
- int ret = -1;
int exitstatus;
*master = NULL;
@@ -438,7 +432,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to run command to get OVS master for "
"interface %s"), ifname);
- goto cleanup;
+ return -1;
}
/* non-0 exit code just means that the interface has no master in OVS */
@@ -454,9 +448,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master :
"(none)");
- ret = 0;
- cleanup:
- return ret;
+ return 0;
Probably should be a separate patch. The rest is fine.
Erik
7:12 a.m.
New subject: [libvirt] [PATCH v1 17/32] util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Fri, 3 Aug 2018 at 19:02, Erik Skultety <eskultet(a)redhat.com> wrote:
On Sat, Jul 28, 2018 at 11:31:32PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> ---
...
> @@ -424,7 +419,6 @@ int
> virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
> {
> virCommandPtr cmd = NULL;
> - int ret = -1;
> int exitstatus;
>
> *master = NULL;
> @@ -438,7 +432,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unable to run command to get OVS master for "
> "interface %s"), ifname);
> - goto cleanup;
> + return -1;
> }
>
> /* non-0 exit code just means that the interface has no master in OVS */
> @@ -454,9 +448,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char
**master)
>
> VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master :
"(none)");
>
> - ret = 0;
> - cleanup:
> - return ret;
> + return 0;
Probably should be a separate patch. The rest is fine.
Another patch just for this function?
3:32 a.m.
New subject: [libvirt] [PATCH v1 17/32] util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sun, Aug 05, 2018 at 05:42:02PM +0530, Sukrit Bhatnagar wrote:
On Fri, 3 Aug 2018 at 19:02, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:32PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOFREE macro for declaring scalar variables, majority
> > of the VIR_FREE calls can be dropped, which in turn leads to
> > getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
> > ---
> ...
>
> > @@ -424,7 +419,6 @@ int
> > virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
> > {
> > virCommandPtr cmd = NULL;
> > - int ret = -1;
> > int exitstatus;
> >
> > *master = NULL;
> > @@ -438,7 +432,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname,
char **master)
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Unable to run command to get OVS master for
"
> > "interface %s"), ifname);
> > - goto cleanup;
> > + return -1;
> > }
> >
> > /* non-0 exit code just means that the interface has no master in OVS */
> > @@ -454,9 +448,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname,
char **master)
> >
> > VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master :
"(none)");
> >
> > - ret = 0;
> > - cleanup:
> > - return ret;
> > + return 0;
>
> Probably should be a separate patch. The rest is fine.
Another patch just for this function?
Yes, that is a change unrelated to the VIR_AUTOFREE effort.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 18/32] util: netdevopenvswitch: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevopenvswitch.c | 106 +++++++++++++++-------------------------
1 file changed, 40 insertions(+), 66 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index a9c5e2a..eae5861 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -76,9 +76,7 @@ virNetDevOpenvswitchAddTimeout(virCommandPtr cmd)
static int
virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr virtVlan)
{
- int ret = -1;
size_t i = 0;
- virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!virtVlan || !virtVlan->nTags)
return 0;
@@ -98,7 +96,12 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr
virtVlan)
}
if (virtVlan->trunk) {
- virBufferAddLit(&buf, "trunk=");
+ VIR_AUTOPTR(virBuffer) buf = NULL;
+
+ if (VIR_ALLOC(buf) < 0)
+ return -1;
+
+ virBufferAddLit(buf, "trunk=");
/*
* Trunk ports have at least one VLAN. Do the first one
@@ -106,24 +109,21 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd,
virNetDevVlanPtr virtVlan)
* start of the for loop if there are more than one VLANs
* on this trunk port.
*/
- virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
+ virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
for (i = 1; i < virtVlan->nTags; i++) {
- virBufferAddLit(&buf, ",");
- virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
+ virBufferAddLit(buf, ",");
+ virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
}
- if (virBufferCheckError(&buf) < 0)
- goto cleanup;
- virCommandAddArg(cmd, virBufferCurrentContent(&buf));
+ if (virBufferCheckError(buf) < 0)
+ return -1;
+ virCommandAddArg(cmd, virBufferCurrentContent(buf));
} else if (virtVlan->nTags) {
virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
}
- ret = 0;
- cleanup:
- virBufferFreeAndReset(&buf);
- return ret;
+ return 0;
}
/**
@@ -144,11 +144,10 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
virNetDevVPortProfilePtr ovsport,
virNetDevVlanPtr virtVlan)
{
- int ret = -1;
- virCommandPtr cmd = NULL;
char macaddrstr[VIR_MAC_STRING_BUFLEN];
char ifuuidstr[VIR_UUID_STRING_BUFLEN];
char vmuuidstr[VIR_UUID_STRING_BUFLEN];
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) attachedmac_ex_id = NULL;
VIR_AUTOFREE(char *) ifaceid_ex_id = NULL;
VIR_AUTOFREE(char *) profile_ex_id = NULL;
@@ -160,17 +159,17 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
if (virAsprintf(&attachedmac_ex_id,
"external-ids:attached-mac=\"%s\"",
macaddrstr) < 0)
- goto cleanup;
+ return -1;
if (virAsprintf(&ifaceid_ex_id,
"external-ids:iface-id=\"%s\"",
ifuuidstr) < 0)
- goto cleanup;
+ return -1;
if (virAsprintf(&vmid_ex_id, "external-ids:vm-id=\"%s\"",
vmuuidstr) < 0)
- goto cleanup;
+ return -1;
if (ovsport->profileID[0] != '\0') {
if (virAsprintf(&profile_ex_id,
"external-ids:port-profile=\"%s\"",
ovsport->profileID) < 0)
- goto cleanup;
+ return -1;
}
cmd = virCommandNew(OVSVSCTL);
@@ -179,7 +178,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
ifname, "--", "add-port", brname, ifname,
NULL);
if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0)
- goto cleanup;
+ return -1;
if (ovsport->profileID[0] == '\0') {
virCommandAddArgList(cmd,
@@ -204,13 +203,10 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to add port %s to OVS bridge %s"),
ifname, brname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
/**
@@ -223,8 +219,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char
*ifname,
*/
int virNetDevOpenvswitchRemovePort(const char *brname ATTRIBUTE_UNUSED, const char
*ifname)
{
- int ret = -1;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNew(OVSVSCTL);
virNetDevOpenvswitchAddTimeout(cmd);
@@ -233,13 +228,10 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to delete port %s from OVS"), ifname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
/**
@@ -253,9 +245,8 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
*/
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
size_t len;
- int ret = -1;
cmd = virCommandNew(OVSVSCTL);
virNetDevOpenvswitchAddTimeout(cmd);
@@ -269,7 +260,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to run command to get OVS port data for "
"interface %s"), ifname);
- goto cleanup;
+ return -1;
}
/* Wipeout the newline, if it exists */
@@ -277,10 +268,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
if (len > 0)
(*migrate)[len - 1] = '\0';
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
/**
@@ -294,8 +282,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
*/
int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname)
{
- virCommandPtr cmd = NULL;
- int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (!migrate) {
VIR_DEBUG("No OVS port data for interface %s", ifname);
@@ -312,13 +299,10 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char
*ifname)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to run command to set OVS port data for "
"interface %s"), ifname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
/**
@@ -334,11 +318,10 @@ int
virNetDevOpenvswitchInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) output = NULL;
char *tmp;
bool gotStats = false;
- int ret = -1;
/* Just ensure the interface exists in ovs */
cmd = virCommandNew(OVSVSCTL);
@@ -350,7 +333,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
/* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Interface not found"));
- goto cleanup;
+ return -1;
}
#define GET_STAT(name, member) \
@@ -369,7 +352,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
*tmp != '\n') { \
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
_("Fail to parse ovs-vsctl output")); \
- goto cleanup; \
+ return -1; \
} \
gotStats = true; \
} \
@@ -389,14 +372,10 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
if (!gotStats) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Interface doesn't have any statistics"));
- goto cleanup;
+ return -1;
}
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -467,7 +446,7 @@ int
virNetDevOpenvswitchGetVhostuserIfname(const char *path,
char **ifname)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) ovs_timeout = NULL;
char *tmpIfname = NULL;
char **tokens = NULL;
@@ -504,7 +483,6 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
cleanup:
virStringListFreeCount(tokens, ntokens);
- virCommandFree(cmd);
return ret;
}
@@ -520,8 +498,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
int virNetDevOpenvswitchUpdateVlan(const char *ifname,
virNetDevVlanPtr virtVlan)
{
- int ret = -1;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNew(OVSVSCTL);
virNetDevOpenvswitchAddTimeout(cmd);
@@ -532,16 +509,13 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname,
"--", "--if-exists", "set",
"Port", ifname, NULL);
if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0)
- goto cleanup;
+ return -1;
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to set vlan configuration on port %s"),
ifname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
--
1.8.3.1
8:35 a.m.
New subject: [libvirt] [PATCH v1 18/32] util: netdevopenvswitch: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:33PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevopenvswitch.c | 106 +++++++++++++++-------------------------
1 file changed, 40 insertions(+), 66 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index a9c5e2a..eae5861 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -76,9 +76,7 @@ virNetDevOpenvswitchAddTimeout(virCommandPtr cmd)
static int
virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr virtVlan)
{
- int ret = -1;
size_t i = 0;
- virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!virtVlan || !virtVlan->nTags)
return 0;
@@ -98,7 +96,12 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr
virtVlan)
}
if (virtVlan->trunk) {
- virBufferAddLit(&buf, "trunk=");
+ VIR_AUTOPTR(virBuffer) buf = NULL;
+
+ if (VIR_ALLOC(buf) < 0)
+ return -1;
+
+ virBufferAddLit(buf, "trunk=");
/*
* Trunk ports have at least one VLAN. Do the first one
@@ -106,24 +109,21 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd,
virNetDevVlanPtr virtVlan)
* start of the for loop if there are more than one VLANs
* on this trunk port.
*/
- virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
+ virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
for (i = 1; i < virtVlan->nTags; i++) {
- virBufferAddLit(&buf, ",");
- virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
+ virBufferAddLit(buf, ",");
+ virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
}
- if (virBufferCheckError(&buf) < 0)
- goto cleanup;
- virCommandAddArg(cmd, virBufferCurrentContent(&buf));
+ if (virBufferCheckError(buf) < 0)
+ return -1;
+ virCommandAddArg(cmd, virBufferCurrentContent(buf));
} else if (virtVlan->nTags) {
virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
}
- ret = 0;
- cleanup:
- virBufferFreeAndReset(&buf);
- return ret;
+ return 0;
The obvious problem with virBuffer :)...otherwise it's fine, + the ordering
issue of the declarations, that applies to all patches.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 19/32] util: netdevtap: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevtap.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index d432577..c2c2e73 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -400,16 +400,14 @@ int virNetDevTapCreate(char **ifname,
if (strstr(*ifname, "%d") != NULL) {
size_t i;
for (i = 0; i <= IF_MAXUNIT; i++) {
- char *newname;
+ VIR_AUTOFREE(char *) newname = NULL;
if (virAsprintf(&newname, *ifname, i) < 0)
goto cleanup;
if (virNetDevExists(newname) == 0) {
- newifname = newname;
+ VIR_STEAL_PTR(newifname, newname);
break;
}
-
- VIR_FREE(newname);
}
if (newifname) {
VIR_FREE(*ifname);
@@ -423,7 +421,7 @@ int virNetDevTapCreate(char **ifname,
}
if (tapfd) {
- char *dev_path = NULL;
+ VIR_AUTOFREE(char *) dev_path = NULL;
if (virAsprintf(&dev_path, "/dev/%s", ifr.ifr_name) < 0)
goto cleanup;
@@ -431,11 +429,8 @@ int virNetDevTapCreate(char **ifname,
virReportSystemError(errno,
_("Unable to open %s"),
dev_path);
- VIR_FREE(dev_path);
goto cleanup;
}
-
- VIR_FREE(dev_path);
}
if (virNetDevSetName(ifr.ifr_name, *ifname) == -1)
--
1.8.3.1
8:37 a.m.
New subject: [libvirt] [PATCH v1 19/32] util: netdevtap: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:34PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 20/32] util: netdevveth: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevveth.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 6905168..8c1a7f3 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -46,12 +46,11 @@ virMutex virNetDevVethCreateMutex = VIR_MUTEX_INITIALIZER;
static int virNetDevVethExists(int devNum)
{
int ret;
- char *path = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAsprintf(&path, SYSFS_NET_DIR "vnet%d/", devNum) < 0)
return -1;
ret = virFileExists(path) ? 1 : 0;
VIR_DEBUG("Checked dev vnet%d usage: %d", devNum, ret);
- VIR_FREE(path);
return ret;
}
@@ -111,8 +110,6 @@ static int virNetDevVethGetFreeNum(int startDev)
int virNetDevVethCreate(char** veth1, char** veth2)
{
int ret = -1;
- char *veth1auto = NULL;
- char *veth2auto = NULL;
int vethNum = 0;
virCommandPtr cmd = NULL;
size_t i;
@@ -125,6 +122,9 @@ int virNetDevVethCreate(char** veth1, char** veth2)
#define MAX_VETH_RETRIES 10
for (i = 0; i < MAX_VETH_RETRIES; i++) {
+ VIR_AUTOFREE(char *) veth1auto = NULL;
+ VIR_AUTOFREE(char *) veth2auto = NULL;
+
int status;
if (!*veth1) {
int veth1num;
@@ -173,8 +173,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*veth1 ? *veth1 : veth1auto,
*veth2 ? *veth2 : veth2auto,
status);
- VIR_FREE(veth1auto);
- VIR_FREE(veth2auto);
virCommandFree(cmd);
cmd = NULL;
}
@@ -186,8 +184,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
cleanup:
virMutexUnlock(&virNetDevVethCreateMutex);
virCommandFree(cmd);
- VIR_FREE(veth1auto);
- VIR_FREE(veth2auto);
return ret;
}
--
1.8.3.1
8:38 a.m.
New subject: [libvirt] [PATCH v1 20/32] util: netdevveth: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:35PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 21/32] util: netdevveth: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevveth.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 8c1a7f3..0b94f73 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -111,7 +111,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
{
int ret = -1;
int vethNum = 0;
- virCommandPtr cmd = NULL;
size_t i;
/*
@@ -124,6 +123,7 @@ int virNetDevVethCreate(char** veth1, char** veth2)
for (i = 0; i < MAX_VETH_RETRIES; i++) {
VIR_AUTOFREE(char *) veth1auto = NULL;
VIR_AUTOFREE(char *) veth2auto = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int status;
if (!*veth1) {
@@ -173,8 +173,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*veth1 ? *veth1 : veth1auto,
*veth2 ? *veth2 : veth2auto,
status);
- virCommandFree(cmd);
- cmd = NULL;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -183,7 +181,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
cleanup:
virMutexUnlock(&virNetDevVethCreateMutex);
- virCommandFree(cmd);
return ret;
}
@@ -200,26 +197,22 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*/
int virNetDevVethDelete(const char *veth)
{
- virCommandPtr cmd = virCommandNewArgList("ip", "link",
"del", veth, NULL);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNewArgList("ip", "link", "del", veth,
NULL);
int status;
- int ret = -1;
if (virCommandRun(cmd, &status) < 0)
- goto cleanup;
+ return -1;
if (status != 0) {
if (!virNetDevExists(veth)) {
VIR_DEBUG("Device %s already deleted (by kernel namespace
cleanup)", veth);
- ret = 0;
- goto cleanup;
+ return 0;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to delete veth device %s"), veth);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
--
1.8.3.1
9:38 a.m.
New subject: [libvirt] [PATCH v1 21/32] util: netdevveth: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:36PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetdevveth.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 8c1a7f3..0b94f73 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -111,7 +111,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
{
int ret = -1;
int vethNum = 0;
- virCommandPtr cmd = NULL;
size_t i;
/*
@@ -124,6 +123,7 @@ int virNetDevVethCreate(char** veth1, char** veth2)
for (i = 0; i < MAX_VETH_RETRIES; i++) {
VIR_AUTOFREE(char *) veth1auto = NULL;
VIR_AUTOFREE(char *) veth2auto = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int status;
if (!*veth1) {
@@ -173,8 +173,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*veth1 ? *veth1 : veth1auto,
*veth2 ? *veth2 : veth2auto,
status);
- virCommandFree(cmd);
- cmd = NULL;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -183,7 +181,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
cleanup:
virMutexUnlock(&virNetDevVethCreateMutex);
- virCommandFree(cmd);
return ret;
}
@@ -200,26 +197,22 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*/
int virNetDevVethDelete(const char *veth)
{
- virCommandPtr cmd = virCommandNewArgList("ip", "link",
"del", veth, NULL);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNewArgList("ip", "link", "del", veth,
NULL);
I'll put this on a single line.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 22/32] util: netlink: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
This commit also adds a typedef for virNetlinkHandlePtr for use with
the cleanup macros.
When a variable of type virNetlinkHandlePtr is declared using
VIR_AUTOPTR, the function virNetlinkFree will be run automatically
on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetlink.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fa1ba3e..8d28387 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -72,6 +72,10 @@ typedef struct nl_handle virNetlinkHandle;
typedef struct nl_sock virNetlinkHandle;
# endif
+typedef virNetlinkHandle *virNetlinkHandlePtr;
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetlinkHandle, virNetlinkFree)
+
typedef struct _virNetlinkEventSrvPrivate virNetlinkEventSrvPrivate;
typedef virNetlinkEventSrvPrivate *virNetlinkEventSrvPrivatePtr;
struct _virNetlinkEventSrvPrivate {
@@ -79,7 +83,7 @@ struct _virNetlinkEventSrvPrivate {
virMutex lock;
int eventwatch;
int netlinkfd;
- virNetlinkHandle *netlinknh;
+ virNetlinkHandlePtr netlinknh;
/*Events*/
int handled;
size_t handlesCount;
@@ -102,7 +106,7 @@ static int nextWatch = 1;
/* Linux kernel supports up to MAX_LINKS (32 at the time) individual
* netlink protocols. */
static virNetlinkEventSrvPrivatePtr server[MAX_LINKS] = {NULL};
-static virNetlinkHandle *placeholder_nlhandle;
+static virNetlinkHandlePtr placeholder_nlhandle;
/* Function definitions */
@@ -172,10 +176,10 @@ virNetlinkShutdown(void)
* Returns a handle to the new netlink socket, or 0 if there was a failure.
*
*/
-static virNetlinkHandle *
+static virNetlinkHandlePtr
virNetlinkCreateSocket(int protocol)
{
- virNetlinkHandle *nlhandle = NULL;
+ virNetlinkHandlePtr nlhandle = NULL;
if (!(nlhandle = virNetlinkAlloc())) {
virReportSystemError(errno, "%s",
@@ -209,7 +213,7 @@ virNetlinkCreateSocket(int protocol)
goto cleanup;
}
-static virNetlinkHandle *
+static virNetlinkHandlePtr
virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
struct sockaddr_nl nladdr,
unsigned int protocol, unsigned int groups)
@@ -217,7 +221,7 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
ssize_t nbytes;
int fd;
int n;
- virNetlinkHandle *nlhandle = NULL;
+ virNetlinkHandlePtr nlhandle = NULL;
struct pollfd fds[1];
struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg);
@@ -303,7 +307,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
.nl_groups = 0,
};
struct pollfd fds[1];
- virNetlinkHandle *nlhandle = NULL;
+ virNetlinkHandlePtr nlhandle = NULL;
int len = 0;
memset(fds, 0, sizeof(fds));
@@ -353,7 +357,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
.nl_pid = dst_pid,
.nl_groups = 0,
};
- virNetlinkHandle *nlhandle = NULL;
+ virNetlinkHandlePtr nlhandle = NULL;
if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr,
protocol, groups)))
--
1.8.3.1
9:18 a.m.
New subject: [libvirt] [PATCH v1 22/32] util: netlink: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:37PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
This commit also adds a typedef for virNetlinkHandlePtr for use with
the cleanup macros.
When a variable of type virNetlinkHandlePtr is declared using
VIR_AUTOPTR, the function virNetlinkFree will be run automatically
on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetlink.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fa1ba3e..8d28387 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -72,6 +72,10 @@ typedef struct nl_handle virNetlinkHandle;
typedef struct nl_sock virNetlinkHandle;
# endif
+typedef virNetlinkHandle *virNetlinkHandlePtr;
Yes, ^this somewhat brings consistency, but it's nothing more than a syntactic
sugar we technically don't need for the attribute cleanup functionality unless
we'll have something like a list of virNetlinkHandles. Although harmless and
correct, I think that it would make much more sense if we simply let the
contributor who's going to add a list of virNetlinkHandles as a NULL-terminated
list in the future to create this typedef as well, it'll make more sense in the
context of such a patch because we'll actually need it, because we wouldn't
have the necessary type for the corresponding *Free method.
+
+VIR_DEFINE_AUTOPTR_FUNC(virNetlinkHandle, virNetlinkFree)
To ^this:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
+
typedef struct _virNetlinkEventSrvPrivate virNetlinkEventSrvPrivate;
typedef virNetlinkEventSrvPrivate *virNetlinkEventSrvPrivatePtr;
struct _virNetlinkEventSrvPrivate {
@@ -79,7 +83,7 @@ struct _virNetlinkEventSrvPrivate {
virMutex lock;
int eventwatch;
int netlinkfd;
- virNetlinkHandle *netlinknh;
+ virNetlinkHandlePtr netlinknh;
^these renames would only add to the noise in the history, we don't benefit
from this rename at all, so all of ^these changes should be dropped
unconditionally...
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 23/32] util: netlink: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetlink.c | 48 +++++++++++++++++++-----------------------------
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 8d28387..6b00559 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -221,30 +221,31 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
ssize_t nbytes;
int fd;
int n;
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
+ virNetlinkHandlePtr temp = NULL;
struct pollfd fds[1];
struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg);
if (protocol >= MAX_LINKS) {
virReportSystemError(EINVAL,
_("invalid protocol argument: %d"), protocol);
- goto error;
+ return NULL;
}
if (!(nlhandle = virNetlinkCreateSocket(protocol)))
- goto error;
+ return NULL;
fd = nl_socket_get_fd(nlhandle);
if (fd < 0) {
virReportSystemError(errno,
"%s", _("cannot get netlink socket
fd"));
- goto error;
+ return NULL;
}
if (groups && nl_socket_add_membership(nlhandle, groups) < 0) {
virReportSystemError(errno,
"%s", _("cannot add netlink
membership"));
- goto error;
+ return NULL;
}
nlmsg_set_dst(nl_msg, &nladdr);
@@ -255,7 +256,7 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
if (nbytes < 0) {
virReportSystemError(errno,
"%s", _("cannot send to netlink
socket"));
- goto error;
+ return NULL;
}
memset(fds, 0, sizeof(fds));
@@ -273,11 +274,9 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
_("no valid netlink response was received"));
}
- return nlhandle;
+ VIR_STEAL_PTR(temp, nlhandle);
- error:
- virNetlinkFree(nlhandle);
- return NULL;
+ return temp;
}
/**
@@ -307,7 +306,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
.nl_groups = 0,
};
struct pollfd fds[1];
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
int len = 0;
memset(fds, 0, sizeof(fds));
@@ -335,7 +334,6 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
*respbuflen = 0;
}
- virNetlinkFree(nlhandle);
return ret;
}
@@ -346,10 +344,8 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
unsigned int protocol, unsigned int groups,
void *opaque)
{
- int ret = -1;
bool end = false;
int len = 0;
- struct nlmsghdr *resp = NULL;
struct nlmsghdr *msg = NULL;
struct sockaddr_nl nladdr = {
@@ -357,13 +353,14 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
.nl_pid = dst_pid,
.nl_groups = 0,
};
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr,
protocol, groups)))
- goto cleanup;
+ return -1;
while (!end) {
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
len = nl_recv(nlhandle, &nladdr, (unsigned char **)&resp, NULL);
VIR_WARNINGS_NO_CAST_ALIGN
for (msg = resp; NLMSG_OK(msg, len); msg = NLMSG_NEXT(msg, len)) {
@@ -372,20 +369,14 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
end = true;
if (virNetlinkGetErrorCode(msg, len) < 0)
- goto cleanup;
+ return -1;
if (callback(msg, opaque) < 0)
- goto cleanup;
+ return -1;
}
- VIR_FREE(resp);
}
- ret = 0;
-
- cleanup:
- VIR_FREE(resp);
- virNetlinkFree(nlhandle);
- return ret;
+ return 0;
}
/**
@@ -527,7 +518,7 @@ int
virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
{
int rc = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
unsigned int recvbuflen;
@@ -582,7 +573,6 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback
fallback)
rc = 0;
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -771,7 +761,7 @@ virNetlinkEventCallback(int watch,
void *opaque)
{
virNetlinkEventSrvPrivatePtr srv = opaque;
- struct nlmsghdr *msg;
+ VIR_AUTOFREE(struct nlmsghdr *) msg = NULL;
struct sockaddr_nl peer;
struct ucred *creds = NULL;
size_t i;
@@ -806,7 +796,7 @@ virNetlinkEventCallback(int watch,
if (!handled)
VIR_DEBUG("event not handled.");
- VIR_FREE(msg);
+
virNetlinkEventServerUnlock(srv);
}
--
1.8.3.1
9:23 a.m.
New subject: [libvirt] [PATCH v1 23/32] util: netlink: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:38PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnetlink.c | 48 +++++++++++++++++++-----------------------------
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 8d28387..6b00559 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -221,30 +221,31 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
ssize_t nbytes;
int fd;
int n;
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
+ virNetlinkHandlePtr temp = NULL;
struct pollfd fds[1];
struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg);
if (protocol >= MAX_LINKS) {
virReportSystemError(EINVAL,
_("invalid protocol argument: %d"), protocol);
- goto error;
+ return NULL;
}
if (!(nlhandle = virNetlinkCreateSocket(protocol)))
- goto error;
+ return NULL;
fd = nl_socket_get_fd(nlhandle);
if (fd < 0) {
virReportSystemError(errno,
"%s", _("cannot get netlink socket
fd"));
- goto error;
+ return NULL;
}
if (groups && nl_socket_add_membership(nlhandle, groups) < 0) {
virReportSystemError(errno,
"%s", _("cannot add netlink
membership"));
- goto error;
+ return NULL;
}
nlmsg_set_dst(nl_msg, &nladdr);
@@ -255,7 +256,7 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
if (nbytes < 0) {
virReportSystemError(errno,
"%s", _("cannot send to netlink
socket"));
- goto error;
+ return NULL;
}
memset(fds, 0, sizeof(fds));
@@ -273,11 +274,9 @@ virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid,
_("no valid netlink response was
received"));
}
- return nlhandle;
+ VIR_STEAL_PTR(temp, nlhandle);
- error:
- virNetlinkFree(nlhandle);
- return NULL;
+ return temp;
}
Similarly to patch 16, we don't need to force the usage of the attribute
cleanup everywhere.
/**
@@ -307,7 +306,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
.nl_groups = 0,
};
struct pollfd fds[1];
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
int len = 0;
memset(fds, 0, sizeof(fds));
@@ -335,7 +334,6 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
*respbuflen = 0;
}
- virNetlinkFree(nlhandle);
return ret;
}
@@ -346,10 +344,8 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
unsigned int protocol, unsigned int groups,
void *opaque)
{
- int ret = -1;
bool end = false;
int len = 0;
- struct nlmsghdr *resp = NULL;
struct nlmsghdr *msg = NULL;
struct sockaddr_nl nladdr = {
@@ -357,13 +353,14 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
.nl_pid = dst_pid,
.nl_groups = 0,
};
- virNetlinkHandlePtr nlhandle = NULL;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr,
protocol, groups)))
- goto cleanup;
+ return -1;
while (!end) {
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
This patch is about VIR_AUTOPTR, therefore ^this should come in a separate
patch.
len = nl_recv(nlhandle, &nladdr, (unsigned char
**)&resp, NULL);
VIR_WARNINGS_NO_CAST_ALIGN
for (msg = resp; NLMSG_OK(msg, len); msg = NLMSG_NEXT(msg, len)) {
@@ -372,20 +369,14 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
end = true;
if (virNetlinkGetErrorCode(msg, len) < 0)
- goto cleanup;
+ return -1;
if (callback(msg, opaque) < 0)
- goto cleanup;
+ return -1;
}
- VIR_FREE(resp);
}
- ret = 0;
-
- cleanup:
- VIR_FREE(resp);
- virNetlinkFree(nlhandle);
- return ret;
+ return 0;
}
/**
@@ -527,7 +518,7 @@ int
virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
{
int rc = -1;
- struct nlmsghdr *resp = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
...here too...
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
unsigned int recvbuflen;
@@ -582,7 +573,6 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback
fallback)
rc = 0;
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -771,7 +761,7 @@ virNetlinkEventCallback(int watch,
void *opaque)
{
virNetlinkEventSrvPrivatePtr srv = opaque;
- struct nlmsghdr *msg;
+ VIR_AUTOFREE(struct nlmsghdr *) msg = NULL;
...and here too...
struct sockaddr_nl peer;
struct ucred *creds = NULL;
size_t i;
@@ -806,7 +796,7 @@ virNetlinkEventCallback(int watch,
if (!handled)
VIR_DEBUG("event not handled.");
- VIR_FREE(msg);
+
virNetlinkEventServerUnlock(srv);
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 24/32] util: numa: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnuma.c | 79 +++++++++++++++++++++---------------------------------
1 file changed, 31 insertions(+), 48 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 784db0a..841c7cb 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -252,8 +252,8 @@ int
virNumaGetNodeCPUs(int node,
virBitmapPtr *cpus)
{
- unsigned long *mask = NULL;
- unsigned long *allonesmask = NULL;
+ VIR_AUTOFREE(unsigned long *) mask = NULL;
+ VIR_AUTOFREE(unsigned long *) allonesmask = NULL;
virBitmapPtr cpumap = NULL;
int ncpus = 0;
int max_n_cpus = virNumaGetMaxCPUs();
@@ -300,8 +300,6 @@ virNumaGetNodeCPUs(int node,
ret = ncpus;
cleanup:
- VIR_FREE(mask);
- VIR_FREE(allonesmask);
virBitmapFree(cpumap);
return ret;
@@ -566,52 +564,47 @@ virNumaGetHugePageInfo(int node,
unsigned long long *page_avail,
unsigned long long *page_free)
{
- int ret = -1;
- char *path = NULL;
- char *buf = NULL;
char *end;
if (page_avail) {
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
if (virNumaGetHugePageInfoPath(&path, node,
page_size, "nr_hugepages") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(buf, &end, 10, page_avail) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
- VIR_FREE(buf);
- VIR_FREE(path);
}
if (page_free) {
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
if (virNumaGetHugePageInfoPath(&path, node,
page_size, "free_hugepages") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(buf, &end, 10, page_free) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
}
- ret = 0;
- cleanup:
- VIR_FREE(buf);
- VIR_FREE(path);
- return ret;
+ return 0;
}
/**
@@ -714,13 +707,13 @@ virNumaGetPages(int node,
size_t *npages)
{
int ret = -1;
- char *path = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(unsigned int *) tmp_size = NULL;
+ VIR_AUTOFREE(unsigned long long *) tmp_avail = NULL;
+ VIR_AUTOFREE(unsigned long long *) tmp_free = NULL;
DIR *dir = NULL;
int direrr = 0;
struct dirent *entry;
- unsigned int *tmp_size = NULL;
- unsigned long long *tmp_avail = NULL;
- unsigned long long *tmp_free = NULL;
unsigned int ntmp = 0;
size_t i;
bool exchange;
@@ -828,11 +821,7 @@ virNumaGetPages(int node,
*npages = ntmp;
ret = 0;
cleanup:
- VIR_FREE(tmp_free);
- VIR_FREE(tmp_avail);
- VIR_FREE(tmp_size);
VIR_DIR_CLOSE(dir);
- VIR_FREE(path);
return ret;
}
@@ -843,8 +832,8 @@ virNumaSetPagePoolSize(int node,
unsigned long long page_count,
bool add)
{
- int ret = -1;
- char *nr_path = NULL, *nr_buf = NULL;
+ VIR_AUTOFREE(char *) nr_path = NULL;
+ VIR_AUTOFREE(char *) nr_buf = NULL;
char *end;
unsigned long long nr_count;
@@ -853,37 +842,35 @@ virNumaSetPagePoolSize(int node,
* differently to huge pages. */
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("system pages pool can't be modified"));
- goto cleanup;
+ return -1;
}
if (virNumaGetHugePageInfoPath(&nr_path, node, page_size,
"nr_hugepages") < 0)
- goto cleanup;
+ return -1;
/* Firstly check, if there's anything for us to do */
if (virFileReadAll(nr_path, 1024, &nr_buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(nr_buf, &end, 10, &nr_count) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_OPERATION_FAILED,
_("invalid number '%s' in '%s'"),
nr_buf, nr_path);
- goto cleanup;
+ return -1;
}
if (add) {
if (!page_count) {
VIR_DEBUG("Nothing left to do: add = true page_count = 0");
- ret = 0;
- goto cleanup;
+ return 0;
}
page_count += nr_count;
} else {
if (nr_count == page_count) {
VIR_DEBUG("Nothing left to do: nr_count = page_count = %llu",
page_count);
- ret = 0;
- goto cleanup;
+ return 0;
}
}
@@ -896,40 +883,36 @@ virNumaSetPagePoolSize(int node,
*/
VIR_FREE(nr_buf);
if (virAsprintf(&nr_buf, "%llu", page_count) < 0)
- goto cleanup;
+ return -1;
if (virFileWriteStr(nr_path, nr_buf, 0) < 0) {
virReportSystemError(errno,
_("Unable to write to: %s"), nr_path);
- goto cleanup;
+ return -1;
}
/* And now do the check. */
VIR_FREE(nr_buf);
if (virFileReadAll(nr_path, 1024, &nr_buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(nr_buf, &end, 10, &nr_count) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_OPERATION_FAILED,
_("invalid number '%s' in '%s'"),
nr_buf, nr_path);
- goto cleanup;
+ return -1;
}
if (nr_count != page_count) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("Unable to allocate %llu pages. Allocated only
%llu"),
page_count, nr_count);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(nr_buf);
- VIR_FREE(nr_path);
- return ret;
+ return 0;
}
--
1.8.3.1
9:35 a.m.
New subject: [libvirt] [PATCH v1 24/32] util: numa: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:39PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnuma.c | 79 +++++++++++++++++++++---------------------------------
1 file changed, 31 insertions(+), 48 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 784db0a..841c7cb 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -252,8 +252,8 @@ int
virNumaGetNodeCPUs(int node,
virBitmapPtr *cpus)
{
- unsigned long *mask = NULL;
- unsigned long *allonesmask = NULL;
+ VIR_AUTOFREE(unsigned long *) mask = NULL;
+ VIR_AUTOFREE(unsigned long *) allonesmask = NULL;
virBitmapPtr cpumap = NULL;
int ncpus = 0;
int max_n_cpus = virNumaGetMaxCPUs();
@@ -300,8 +300,6 @@ virNumaGetNodeCPUs(int node,
ret = ncpus;
cleanup:
- VIR_FREE(mask);
- VIR_FREE(allonesmask);
virBitmapFree(cpumap);
return ret;
@@ -566,52 +564,47 @@ virNumaGetHugePageInfo(int node,
unsigned long long *page_avail,
unsigned long long *page_free)
{
- int ret = -1;
- char *path = NULL;
- char *buf = NULL;
char *end;
if (page_avail) {
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
I don't believe this VIR_AUTOFREE duplication is necessary, just declare it at
the function level...
if (virNumaGetHugePageInfoPath(&path, node,
page_size, "nr_hugepages") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(buf, &end, 10, page_avail) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
- VIR_FREE(buf);
- VIR_FREE(path);
... and leave ^these two in place.
}
if (page_free) {
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
if (virNumaGetHugePageInfoPath(&path, node,
page_size, "free_hugepages") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0)
- goto cleanup;
+ return -1;
if (virStrToLong_ull(buf, &end, 10, page_free) < 0 ||
*end != '\n') {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
}
I'll fix those locally and merge.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 25/32] util: numa: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnuma.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 841c7cb..fde46ec 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -57,7 +57,7 @@ char *
virNumaGetAutoPlacementAdvice(unsigned short vcpus,
unsigned long long balloon)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
char *output = NULL;
cmd = virCommandNewArgList(NUMAD, "-w", NULL);
@@ -71,7 +71,6 @@ virNumaGetAutoPlacementAdvice(unsigned short vcpus,
_("Failed to query numad for the "
"advisory nodeset"));
- virCommandFree(cmd);
return output;
}
#else /* !HAVE_NUMAD */
@@ -252,41 +251,38 @@ int
virNumaGetNodeCPUs(int node,
virBitmapPtr *cpus)
{
+ VIR_AUTOPTR(virBitmap) cpumap = NULL;
VIR_AUTOFREE(unsigned long *) mask = NULL;
VIR_AUTOFREE(unsigned long *) allonesmask = NULL;
- virBitmapPtr cpumap = NULL;
int ncpus = 0;
int max_n_cpus = virNumaGetMaxCPUs();
int mask_n_bytes = max_n_cpus / 8;
size_t i;
- int ret = -1;
*cpus = NULL;
if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof(*mask)) < 0)
- goto cleanup;
+ return -1;
if (VIR_ALLOC_N(allonesmask, mask_n_bytes / sizeof(*mask)) < 0)
- goto cleanup;
+ return -1;
memset(allonesmask, 0xff, mask_n_bytes);
/* The first time this returns -1, ENOENT if node doesn't exist... */
if (numa_node_to_cpus(node, mask, mask_n_bytes) < 0) {
VIR_WARN("NUMA topology for cell %d is not available, ignoring",
node);
- ret = -2;
- goto cleanup;
+ return -2;
}
/* second, third... times it returns an all-1's mask */
if (memcmp(mask, allonesmask, mask_n_bytes) == 0) {
VIR_DEBUG("NUMA topology for cell %d is invalid, ignoring", node);
- ret = -2;
- goto cleanup;
+ return -2;
}
if (!(cpumap = virBitmapNew(max_n_cpus)))
- goto cleanup;
+ return -1;
for (i = 0; i < max_n_cpus; i++) {
if (MASK_CPU_ISSET(mask, i)) {
@@ -295,14 +291,8 @@ virNumaGetNodeCPUs(int node,
}
}
- *cpus = cpumap;
- cpumap = NULL;
- ret = ncpus;
-
- cleanup:
- virBitmapFree(cpumap);
-
- return ret;
+ VIR_STEAL_PTR(*cpus, cpumap);
+ return ncpus;
}
# undef MASK_CPU_ISSET
# undef n_bits
--
1.8.3.1
9:36 a.m.
New subject: [libvirt] [PATCH v1 25/32] util: numa: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:40PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virnuma.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 841c7cb..fde46ec 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -57,7 +57,7 @@ char *
virNumaGetAutoPlacementAdvice(unsigned short vcpus,
unsigned long long balloon)
{
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
I'll fix the ordering as I mentioned earlier and merge.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 26/32] util: perf: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virPerfPtr is declared using VIR_AUTOPTR,
the function virPerfFree will be run automatically on it when it
goes out of scope.
This commit also adds an intermediate typedef for virPerf
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virperf.c | 3 +--
src/util/virperf.h | 8 ++++++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/util/virperf.c b/src/util/virperf.c
index 2c832b3..4537cd0 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -26,7 +26,6 @@
#endif
#include "virperf.h"
-#include "viralloc.h"
#include "virerror.h"
#include "virlog.h"
#include "virfile.h"
@@ -61,7 +60,7 @@ struct virPerfEvent {
};
typedef struct virPerfEvent *virPerfEventPtr;
-struct virPerf {
+struct _virPerf {
struct virPerfEvent events[VIR_PERF_EVENT_LAST];
};
diff --git a/src/util/virperf.h b/src/util/virperf.h
index eee7a03..9d0d5ac 100644
--- a/src/util/virperf.h
+++ b/src/util/virperf.h
@@ -23,6 +23,7 @@
# define __VIR_PERF_H__
# include "virutil.h"
+# include "viralloc.h"
/* Some Intel processor families introduced some RDT (Resource Director
* Technology) features to monitor or control shared resource based on
@@ -62,8 +63,9 @@ typedef enum {
VIR_ENUM_DECL(virPerfEvent);
-struct virPerf;
-typedef struct virPerf *virPerfPtr;
+struct _virPerf;
+typedef struct _virPerf virPerf;
+typedef virPerf *virPerfPtr;
virPerfPtr virPerfNew(void);
@@ -83,4 +85,6 @@ int virPerfReadEvent(virPerfPtr perf,
virPerfEventType type,
uint64_t *value);
+VIR_DEFINE_AUTOPTR_FUNC(virPerf, virPerfFree)
+
#endif /* __VIR_PERF_H__ */
--
1.8.3.1
1:24 a.m.
New subject: [libvirt] [PATCH v1 26/32] util: perf: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:41PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virPerfPtr is declared using VIR_AUTOPTR,
the function virPerfFree will be run automatically on it when it
goes out of scope.
This commit also adds an intermediate typedef for virPerf
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 27/32] util: perf: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virperf.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/src/util/virperf.c b/src/util/virperf.c
index 4537cd0..1203a6e 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -175,12 +175,12 @@ typedef struct virPerfEventAttr *virPerfEventAttrPtr;
static int
virPerfRdtAttrInit(void)
{
- char *buf = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
char *tmp = NULL;
unsigned int attr_type = 0;
if (virFileReadAllQuiet("/sys/devices/intel_cqm/type", 10, &buf) <
0)
- goto error;
+ return -1;
if ((tmp = strchr(buf, '\n')))
*tmp = '\0';
@@ -188,19 +188,14 @@ virPerfRdtAttrInit(void)
if (virStrToLong_ui(buf, NULL, 10, &attr_type) < 0) {
virReportSystemError(errno, "%s",
_("failed to get rdt event type"));
- goto error;
+ return -1;
}
- VIR_FREE(buf);
attrs[VIR_PERF_EVENT_CMT].attrType = attr_type;
attrs[VIR_PERF_EVENT_MBMT].attrType = attr_type;
attrs[VIR_PERF_EVENT_MBML].attrType = attr_type;
return 0;
-
- error:
- VIR_FREE(buf);
- return -1;
}
@@ -209,7 +204,6 @@ virPerfEventEnable(virPerfPtr perf,
virPerfEventType type,
pid_t pid)
{
- char *buf = NULL;
struct perf_event_attr attr;
virPerfEventPtr event = &(perf->events[type]);
virPerfEventAttrPtr event_attr = &attrs[type];
@@ -227,6 +221,8 @@ virPerfEventEnable(virPerfPtr perf,
}
if (type == VIR_PERF_EVENT_CMT) {
+ VIR_AUTOFREE(char *) buf = NULL;
+
if
(virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale",
10, &buf) < 0)
goto error;
@@ -236,8 +232,6 @@ virPerfEventEnable(virPerfPtr perf,
_("failed to get cmt scaling factor"));
goto error;
}
-
- VIR_FREE(buf);
}
memset(&attr, 0, sizeof(attr));
@@ -268,7 +262,6 @@ virPerfEventEnable(virPerfPtr perf,
error:
VIR_FORCE_CLOSE(event->fd);
- VIR_FREE(buf);
return -1;
}
--
1.8.3.1
1:25 a.m.
New subject: [libvirt] [PATCH v1 27/32] util: perf: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:42PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virperf.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/src/util/virperf.c b/src/util/virperf.c
index 4537cd0..1203a6e 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -175,12 +175,12 @@ typedef struct virPerfEventAttr *virPerfEventAttrPtr;
static int
virPerfRdtAttrInit(void)
{
- char *buf = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
I'll move this at the end of the declaration block.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 28/32] util: pidfile: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virpidfile.c | 185 ++++++++++++++++----------------------------------
1 file changed, 59 insertions(+), 126 deletions(-)
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 1a85d43..d82bf92 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -97,29 +97,18 @@ int virPidFileWrite(const char *dir,
const char *name,
pid_t pid)
{
- int rc;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (virFileMakePath(dir) < 0) {
- rc = -errno;
- goto cleanup;
- }
+ if (virFileMakePath(dir) < 0)
+ return -errno;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileWritePath(pidfile, pid);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileWritePath(pidfile, pid);
}
@@ -170,25 +159,16 @@ int virPidFileRead(const char *dir,
const char *name,
pid_t *pid)
{
- int rc;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
*pid = 0;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileReadPath(pidfile, pid);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileReadPath(pidfile, pid);
}
@@ -219,11 +199,11 @@ int virPidFileReadPathIfAlive(const char *path,
{
int ret;
bool isLink;
- char *procPath = NULL;
- char *procLink = NULL;
+ VIR_AUTOFREE(char *) procPath = NULL;
+ VIR_AUTOFREE(char *) procLink = NULL;
+ VIR_AUTOFREE(char *) resolvedBinPath = NULL;
+ VIR_AUTOFREE(char *) resolvedProcLink = NULL;
size_t procLinkLen;
- char *resolvedBinPath = NULL;
- char *resolvedProcLink = NULL;
const char deletedText[] = " (deleted)";
size_t deletedTextLen = strlen(deletedText);
pid_t retPid;
@@ -232,7 +212,7 @@ int virPidFileReadPathIfAlive(const char *path,
*pid = -1;
if ((ret = virPidFileReadPath(path, &retPid)) < 0)
- goto cleanup;
+ return ret;
#ifndef WIN32
/* Check that it's still alive. Safe to skip this sanity check on
@@ -252,13 +232,12 @@ int virPidFileReadPathIfAlive(const char *path,
goto cleanup;
}
- if (virAsprintf(&procPath, "/proc/%lld/exe", (long long)retPid) < 0)
{
- ret = -ENOMEM;
- goto cleanup;
- }
+ if (virAsprintf(&procPath, "/proc/%lld/exe", (long long)retPid) <
0)
+ return -ENOMEM;
if ((ret = virFileIsLink(procPath)) < 0)
- goto cleanup;
+ return ret;
+
isLink = ret;
if (isLink && virFileLinkPointsTo(procPath, binPath)) {
@@ -275,27 +254,21 @@ int virPidFileReadPathIfAlive(const char *path,
* "$procpath (deleted)". Read that link, remove the "
(deleted)"
* part, and see if it has the same canonicalized name as binpath.
*/
- if (!(procLink = areadlink(procPath))) {
- ret = -errno;
- goto cleanup;
- }
+ if (!(procLink = areadlink(procPath)))
+ return -errno;
+
procLinkLen = strlen(procLink);
if (procLinkLen > deletedTextLen)
procLink[procLinkLen - deletedTextLen] = 0;
if ((ret = virFileResolveAllLinks(binPath, &resolvedBinPath)) < 0)
- goto cleanup;
+ return ret;
if ((ret = virFileResolveAllLinks(procLink, &resolvedProcLink)) < 0)
- goto cleanup;
+ return ret;
ret = STREQ(resolvedBinPath, resolvedProcLink) ? 0 : -1;
cleanup:
- VIR_FREE(procPath);
- VIR_FREE(procLink);
- VIR_FREE(resolvedProcLink);
- VIR_FREE(resolvedBinPath);
-
/* return the originally set pid of -1 unless we proclaim success */
if (ret == 0)
*pid = retPid;
@@ -326,24 +299,15 @@ int virPidFileReadIfAlive(const char *dir,
pid_t *pid,
const char *binpath)
{
- int rc = 0;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileReadPathIfAlive(pidfile, pid, binpath);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileReadPathIfAlive(pidfile, pid, binpath);
}
@@ -361,24 +325,15 @@ int virPidFileDeletePath(const char *pidfile)
int virPidFileDelete(const char *dir,
const char *name)
{
- int rc = 0;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileDeletePath(pidfile);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileDeletePath(pidfile);
}
int virPidFileAcquirePath(const char *path,
@@ -470,24 +425,15 @@ int virPidFileAcquire(const char *dir,
bool waitForLock,
pid_t pid)
{
- int rc = 0;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileAcquirePath(pidfile, waitForLock, pid);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileAcquirePath(pidfile, waitForLock, pid);
}
@@ -518,24 +464,15 @@ int virPidFileRelease(const char *dir,
const char *name,
int fd)
{
- int rc = 0;
- char *pidfile = NULL;
+ VIR_AUTOFREE(char *) pidfile = NULL;
- if (name == NULL || dir == NULL) {
- rc = -EINVAL;
- goto cleanup;
- }
+ if (name == NULL || dir == NULL)
+ return -EINVAL;
- if (!(pidfile = virPidFileBuildPath(dir, name))) {
- rc = -ENOMEM;
- goto cleanup;
- }
+ if (!(pidfile = virPidFileBuildPath(dir, name)))
+ return -ENOMEM;
- rc = virPidFileReleasePath(pidfile, fd);
-
- cleanup:
- VIR_FREE(pidfile);
- return rc;
+ return virPidFileReleasePath(pidfile, fd);
}
@@ -545,8 +482,7 @@ virPidFileConstructPath(bool privileged,
const char *progname,
char **pidfile)
{
- int ret = -1;
- char *rundir = NULL;
+ VIR_AUTOFREE(char *) rundir = NULL;
if (privileged) {
/*
@@ -556,29 +492,26 @@ virPidFileConstructPath(bool privileged,
if (!statedir) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("No statedir specified"));
- goto cleanup;
+ return -1;
}
if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0)
- goto cleanup;
+ return -1;
} else {
if (!(rundir = virGetUserRuntimeDirectory()))
- goto cleanup;
+ return -1;
if (virFileMakePathWithMode(rundir, 0700) < 0) {
virReportSystemError(errno,
_("Cannot create user runtime directory
'%s'"),
rundir);
- goto cleanup;
+ return -1;
}
if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0)
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(rundir);
- return ret;
+ return 0;
}
--
1.8.3.1
1:27 a.m.
New subject: [libvirt] [PATCH v1 28/32] util: pidfile: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:43PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
...
@@ -219,11 +199,11 @@ int virPidFileReadPathIfAlive(const char *path,
{
int ret;
bool isLink;
- char *procPath = NULL;
- char *procLink = NULL;
+ VIR_AUTOFREE(char *) procPath = NULL;
+ VIR_AUTOFREE(char *) procLink = NULL;
+ VIR_AUTOFREE(char *) resolvedBinPath = NULL;
+ VIR_AUTOFREE(char *) resolvedProcLink = NULL;
I'll move ^this at the end of the section.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 29/32] util: process: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virprocess.c | 49 ++++++++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index f92b0dc..1dd14de 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -158,7 +158,7 @@ virProcessAbort(pid_t pid)
int saved_errno;
int ret;
int status;
- char *tmp = NULL;
+ VIR_AUTOFREE(char *) tmp = NULL;
if (pid <= 0)
return;
@@ -199,7 +199,6 @@ virProcessAbort(pid_t pid)
VIR_DEBUG("failed to reap child %lld, abandoning it", (long long) pid);
cleanup:
- VIR_FREE(tmp);
errno = saved_errno;
}
#else
@@ -271,11 +270,10 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
error:
{
- char *st = virProcessTranslateStatus(status);
+ VIR_AUTOFREE(char *) st = virProcessTranslateStatus(status);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Child process (%lld) unexpected %s"),
(long long) pid, NULLSTR(st));
- VIR_FREE(st);
}
return -1;
}
@@ -475,7 +473,11 @@ virBitmapPtr
virProcessGetAffinity(pid_t pid)
{
size_t i;
+# ifdef CPU_ALLOC
cpu_set_t *mask;
+# else
+ VIR_AUTOFREE(cpu_set_t *) mask = NULL;
+# endif
size_t masklen;
size_t ncpus;
virBitmapPtr ret = NULL;
@@ -504,11 +506,20 @@ virProcessGetAffinity(pid_t pid)
if (sched_getaffinity(pid, masklen, mask) < 0) {
virReportSystemError(errno,
_("cannot get CPU affinity of process %d"), pid);
+# ifdef CPU_ALLOC
goto cleanup;
+# else
+ return ret;
+# endif
}
if (!(ret = virBitmapNew(ncpus)))
- goto cleanup;
+# ifdef CPU_ALLOC
+
+ goto cleanup;
+# else
+ return ret;
+# endif
for (i = 0; i < ncpus; i++) {
# ifdef CPU_ALLOC
@@ -522,11 +533,7 @@ virProcessGetAffinity(pid_t pid)
}
cleanup:
-# ifdef CPU_ALLOC
CPU_FREE(mask);
-# else
- VIR_FREE(mask);
-# endif
return ret;
}
@@ -603,7 +610,7 @@ virProcessGetAffinity(pid_t pid ATTRIBUTE_UNUSED)
int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
{
int ret = -1;
- char *taskPath = NULL;
+ VIR_AUTOFREE(char *) taskPath = NULL;
DIR *dir = NULL;
int value;
struct dirent *ent;
@@ -636,7 +643,6 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
cleanup:
VIR_DIR_CLOSE(dir);
- VIR_FREE(taskPath);
if (ret < 0)
VIR_FREE(*pids);
return ret;
@@ -648,7 +654,6 @@ int virProcessGetNamespaces(pid_t pid,
int **fdlist)
{
int ret = -1;
- char *nsfile = NULL;
size_t i = 0;
const char *ns[] = { "user", "ipc", "uts",
"net", "pid", "mnt" };
@@ -656,6 +661,7 @@ int virProcessGetNamespaces(pid_t pid,
*fdlist = NULL;
for (i = 0; i < ARRAY_CARDINALITY(ns); i++) {
+ VIR_AUTOFREE(char *) nsfile = NULL;
int fd;
if (virAsprintf(&nsfile, "/proc/%llu/ns/%s",
@@ -671,14 +677,11 @@ int virProcessGetNamespaces(pid_t pid,
(*fdlist)[(*nfdlist)-1] = fd;
}
-
- VIR_FREE(nsfile);
}
ret = 0;
cleanup:
- VIR_FREE(nsfile);
if (ret < 0) {
for (i = 0; i < *nfdlist; i++)
VIR_FORCE_CLOSE((*fdlist)[i]);
@@ -977,8 +980,8 @@ virProcessSetMaxCoreSize(pid_t pid ATTRIBUTE_UNUSED,
int virProcessGetStartTime(pid_t pid,
unsigned long long *timestamp)
{
- char *filename = NULL;
- char *buf = NULL;
+ VIR_AUTOFREE(char *) filename = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
char *tmp;
int ret = -1;
int len;
@@ -1032,8 +1035,6 @@ int virProcessGetStartTime(pid_t pid,
cleanup:
virStringListFree(tokens);
- VIR_FREE(filename);
- VIR_FREE(buf);
return ret;
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
@@ -1080,7 +1081,7 @@ static int virProcessNamespaceHelper(int errfd,
virProcessNamespaceCallback cb,
void *opaque)
{
- char *path;
+ VIR_AUTOFREE(char *) path = NULL;
int fd = -1;
int ret = -1;
@@ -1109,7 +1110,6 @@ static int virProcessNamespaceHelper(int errfd,
ignore_value(safewrite(errfd, err->message, len));
}
}
- VIR_FREE(path);
VIR_FORCE_CLOSE(fd);
return ret;
}
@@ -1145,7 +1145,7 @@ virProcessRunInMountNamespace(pid_t pid,
VIR_FORCE_CLOSE(errfd[1]);
_exit(ret < 0 ? EXIT_CANCELED : ret);
} else {
- char *buf = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
int status;
VIR_FORCE_CLOSE(errfd[1]);
@@ -1159,7 +1159,6 @@ virProcessRunInMountNamespace(pid_t pid,
NULLSTR(buf));
}
}
- VIR_FREE(buf);
}
cleanup:
@@ -1226,7 +1225,7 @@ virProcessNamespaceAvailable(unsigned int ns)
int flags = 0;
int cpid;
char *childStack;
- char *stack;
+ VIR_AUTOFREE(char *)stack = NULL;
int stacksize = getpagesize() * 4;
if (ns & VIR_PROCESS_NAMESPACE_MNT)
@@ -1251,7 +1250,7 @@ virProcessNamespaceAvailable(unsigned int ns)
childStack = stack + stacksize;
cpid = clone(virProcessDummyChild, childStack, flags, NULL);
- VIR_FREE(stack);
+
if (cpid < 0) {
char ebuf[1024] ATTRIBUTE_UNUSED;
VIR_DEBUG("clone call returned %s, container support is not enabled",
--
1.8.3.1
1:42 a.m.
New subject: [libvirt] [PATCH v1 29/32] util: process: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:44PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virprocess.c | 49 ++++++++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 25 deletions(-)
@@ -475,7 +473,11 @@ virBitmapPtr
virProcessGetAffinity(pid_t pid)
{
size_t i;
+# ifdef CPU_ALLOC
cpu_set_t *mask;
+# else
+ VIR_AUTOFREE(cpu_set_t *) mask = NULL;
+# endif
size_t masklen;
size_t ncpus;
virBitmapPtr ret = NULL;
@@ -504,11 +506,20 @@ virProcessGetAffinity(pid_t pid)
if (sched_getaffinity(pid, masklen, mask) < 0) {
virReportSystemError(errno,
_("cannot get CPU affinity of process %d"),
pid);
+# ifdef CPU_ALLOC
goto cleanup;
+# else
+ return ret;
+# endif
}
if (!(ret = virBitmapNew(ncpus)))
- goto cleanup;
+# ifdef CPU_ALLOC
+
+ goto cleanup;
+# else
+ return ret;
+# endif
for (i = 0; i < ncpus; i++) {
# ifdef CPU_ALLOC
@@ -522,11 +533,7 @@ virProcessGetAffinity(pid_t pid)
}
cleanup:
-# ifdef CPU_ALLOC
CPU_FREE(mask);
-# else
- VIR_FREE(mask);
-# endif
So instead of 1 conditional compile-time block we'd end up with 3, I don't
think that's a good idea, better leave it as is. We could potentially
conditionally define VIR_DEFINE_AUTOPTR_FUNC handling these both cases, however
I don't particularly like that approach either, so let's leave this hunk out.
To the rest:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:55 a.m.
New subject: [libvirt] [PATCH v1 29/32] util: process: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:44PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virprocess.c | 49 ++++++++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 25 deletions(-)
...
@@ -271,11 +270,10 @@ virProcessWait(pid_t pid, int *exitstatus, bool
raw)
error:
{
- char *st = virProcessTranslateStatus(status);
+ VIR_AUTOFREE(char *) st = virProcessTranslateStatus(status);
I also moved the declaration at the function block level, thus being able to
drop the curly braces, as that's a style we don't use anymore.
Erik
1:01 p.m.
New subject: [libvirt] [PATCH v1 30/32] util: process: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virprocess.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 1dd14de..b51d899 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -982,16 +982,15 @@ int virProcessGetStartTime(pid_t pid,
{
VIR_AUTOFREE(char *) filename = NULL;
VIR_AUTOFREE(char *) buf = NULL;
+ VIR_AUTOPTR(virString) tokens = NULL;
char *tmp;
- int ret = -1;
int len;
- char **tokens = NULL;
if (virAsprintf(&filename, "/proc/%llu/stat", (long long) pid) < 0)
return -1;
if ((len = virFileReadAll(filename, 1024, &buf)) < 0)
- goto cleanup;
+ return -1;
/* start time is the token at index 19 after the '(process name)' entry -
since only this
* field can contain the ')' character, search backwards for this to avoid
malicious
@@ -1002,14 +1001,14 @@ int virProcessGetStartTime(pid_t pid,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot find start time in %s"),
filename);
- goto cleanup;
+ return -1;
}
tmp += 2; /* skip ') ' */
if ((tmp - buf) >= len) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot find start time in %s"),
filename);
- goto cleanup;
+ return -1;
}
tokens = virStringSplit(tmp, " ", 0);
@@ -1018,7 +1017,7 @@ int virProcessGetStartTime(pid_t pid,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot find start time in %s"),
filename);
- goto cleanup;
+ return -1;
}
if (virStrToLong_ull(tokens[19],
@@ -1028,14 +1027,10 @@ int virProcessGetStartTime(pid_t pid,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot parse start time %s in %s"),
tokens[19], filename);
- goto cleanup;
+ return -1;
}
- ret = 0;
-
- cleanup:
- virStringListFree(tokens);
- return ret;
+ return 0;
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
int virProcessGetStartTime(pid_t pid,
--
1.8.3.1
1:43 a.m.
New subject: [libvirt] [PATCH v1 30/32] util: process: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:45PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virprocess.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 1dd14de..b51d899 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -982,16 +982,15 @@ int virProcessGetStartTime(pid_t pid,
{
VIR_AUTOFREE(char *) filename = NULL;
VIR_AUTOFREE(char *) buf = NULL;
+ VIR_AUTOPTR(virString) tokens = NULL;
I'll take care of moving this.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 31/32] util: qemu: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virqemu.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 30b8dc1..4089b8e 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -85,29 +85,22 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key,
virBufferPtr buf)
{
virJSONValuePtr member;
- char *prefix = NULL;
size_t i;
- int ret = 0;
for (i = 0; i < virJSONValueArraySize(array); i++) {
+ VIR_AUTOFREE(char *) prefix = NULL;
member = virJSONValueArrayGet((virJSONValuePtr) array, i);
if (virAsprintf(&prefix, "%s.%zu", key, i) < 0)
- goto cleanup;
+ return 0;
if (virQEMUBuildCommandLineJSONRecurse(prefix, member, buf,
virQEMUBuildCommandLineJSONArrayNumbered,
true) < 0)
- goto cleanup;
-
- VIR_FREE(prefix);
+ return 0;
}
- ret = 0;
-
- cleanup:
- VIR_FREE(prefix);
- return ret;
+ return 0;
}
@@ -118,23 +111,18 @@ virQEMUBuildCommandLineJSONIterate(const char *key,
void *opaque)
{
struct virQEMUCommandLineJSONIteratorData *data = opaque;
- char *tmpkey = NULL;
- int ret = -1;
if (data->prefix) {
+ VIR_AUTOFREE(char *) tmpkey = NULL;
if (virAsprintf(&tmpkey, "%s.%s", data->prefix, key) < 0)
return -1;
- ret = virQEMUBuildCommandLineJSONRecurse(tmpkey, value, data->buf,
+ return virQEMUBuildCommandLineJSONRecurse(tmpkey, value, data->buf,
data->arrayFunc, false);
-
- VIR_FREE(tmpkey);
} else {
- ret = virQEMUBuildCommandLineJSONRecurse(key, value, data->buf,
+ return virQEMUBuildCommandLineJSONRecurse(key, value, data->buf,
data->arrayFunc, false);
}
-
- return ret;
}
--
1.8.3.1
3:51 a.m.
New subject: [libvirt] [PATCH v1 31/32] util: qemu: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:46PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1:01 p.m.
New subject: [libvirt] [PATCH v1 32/32] util: qemu: use VIR_AUTOPTR for aggregate types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virqemu.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 4089b8e..cb42d38 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -56,7 +56,7 @@ virQEMUBuildCommandLineJSONArrayBitmap(const char *key,
{
ssize_t pos = -1;
ssize_t end;
- virBitmapPtr bitmap = NULL;
+ VIR_AUTOPTR(virBitmap) bitmap = NULL;
if (virJSONValueGetArrayAsBitmap(array, &bitmap) < 0)
return -1;
@@ -73,8 +73,6 @@ virQEMUBuildCommandLineJSONArrayBitmap(const char *key,
}
}
- virBitmapFree(bitmap);
-
return 0;
}
@@ -267,21 +265,19 @@ virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf,
char *
virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr srcdef)
{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
- char *ret = NULL;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
- if (virQEMUBuildCommandLineJSON(srcdef, &buf,
+ if (VIR_ALLOC(buf) < 0)
+ return NULL;
+
+ if (virQEMUBuildCommandLineJSON(srcdef, buf,
virQEMUBuildCommandLineJSONArrayNumbered) < 0)
- goto cleanup;
+ return NULL;
- if (virBufferCheckError(&buf) < 0)
- goto cleanup;
+ if (virBufferCheckError(buf) < 0)
+ return NULL;
- ret = virBufferContentAndReset(&buf);
-
- cleanup:
- virBufferFreeAndReset(&buf);
- return ret;
+ return virBufferContentAndReset(buf);
}
--
1.8.3.1
3:52 a.m.
New subject: [libvirt] [PATCH v1 32/32] util: qemu: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virqemu.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 4089b8e..cb42d38 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -56,7 +56,7 @@ virQEMUBuildCommandLineJSONArrayBitmap(const char *key,
{
ssize_t pos = -1;
ssize_t end;
- virBitmapPtr bitmap = NULL;
+ VIR_AUTOPTR(virBitmap) bitmap = NULL;
if (virJSONValueGetArrayAsBitmap(array, &bitmap) < 0)
return -1;
@@ -73,8 +73,6 @@ virQEMUBuildCommandLineJSONArrayBitmap(const char *key,
}
}
- virBitmapFree(bitmap);
-
return 0;
}
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
@@ -267,21 +265,19 @@ virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf,
char *
virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr srcdef)
{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
- char *ret = NULL;
+ VIR_AUTOPTR(virBuffer) buf = NULL;
- if (virQEMUBuildCommandLineJSON(srcdef, &buf,
+ if (VIR_ALLOC(buf) < 0)
+ return NULL;
+
+ if (virQEMUBuildCommandLineJSON(srcdef, buf,
virQEMUBuildCommandLineJSONArrayNumbered) < 0)
- goto cleanup;
+ return NULL;
- if (virBufferCheckError(&buf) < 0)
- goto cleanup;
+ if (virBufferCheckError(buf) < 0)
+ return NULL;
- ret = virBufferContentAndReset(&buf);
-
- cleanup:
- virBufferFreeAndReset(&buf);
- return ret;
+ return virBufferContentAndReset(buf);
we talked about virBuffer in previous patches, so that will need fixing,
I'll push the hunk above.
Erik
9:35 a.m.
On Sat, Jul 28, 2018 at 11:31:15PM +0530, Sukrit Bhatnagar wrote:
This third series of patches also modifies a few files in src/util
to use VIR_AUTOFREE and VIR_AUTOPTR for automatic freeing of memory
and get rid of some VIR_FREE macro invocations and *Free function
calls.
So I went ahead an pushed most of the patches (some got tiny adjustments as I
noted in the review, I split some of them in 2 when there was a function being
defined at the same time as a usage of the VIR_DEFINE_AUTOPTR_FUNC).
Following are the patches where there were some issues that need to be worked
on (like the virBuffer leak) or there was something missing and therefore are
not pushed: 3, 6, 7, 10, 13, 14, 18, 23.
Erik
2301
days inactive
2311
days old
84 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety -
Sukrit Bhatnagar