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@gmail.com>
---
src/util/virnetlink.c | 72 ++++++++++++++++++++-------------------------------
1 file changed, 28 insertions(+), 44 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fcdc09d..66e80e2 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -297,15 +297,16 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
uint32_t src_pid, uint32_t dst_pid,
unsigned int protocol, unsigned int groups)
{
- int ret = -1;
struct sockaddr_nl nladdr = {
.nl_family = AF_NETLINK,
.nl_pid = dst_pid,
.nl_groups = 0,
};
struct pollfd fds[1];
- VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
int len = 0;
+ VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL;
+
+ *respbuflen = 0;
memset(fds, 0, sizeof(fds));
@@ -324,15 +325,12 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
goto cleanup;
}
- ret = 0;
*respbuflen = len;
- cleanup:
- if (ret < 0) {
- *resp = NULL;
- *respbuflen = 0;
- }
+ return 0;
- return ret;
+ cleanup:
+ *resp = NULL;
+ return -1;
}
int
@@ -409,7 +407,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
.ifi_index = ifindex
};
unsigned int recvbuflen;
- struct nl_msg *nl_msg;
+ VIR_AUTOPTR(virNlMsg) nl_msg = NULL;
VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
*nlData = NULL;
@@ -450,7 +448,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
- goto cleanup;
+ return -1;
if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
goto malformed_resp;
@@ -465,7 +463,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
virReportSystemError(-err->error,
_("error dumping %s (%d) interface"),
ifname, ifindex);
- goto cleanup;
+ return -1;
}
break;
@@ -482,21 +480,17 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
}
VIR_STEAL_PTR(*nlData, resp);
- rc = 0;
-
- cleanup:
- nlmsg_free(nl_msg);
- return rc;
+ return 0;
malformed_resp:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- goto cleanup;
+ return rc;
buffer_too_small:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- goto cleanup;
+ return rc;
}
@@ -518,11 +512,10 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
int
virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
{
- int rc = -1;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
unsigned int recvbuflen;
- struct nl_msg *nl_msg;
+ VIR_AUTOPTR(virNlMsg) nl_msg = NULL;
VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
nl_msg = nlmsg_alloc_simple(RTM_DELLINK,
@@ -540,7 +533,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
NETLINK_ROUTE, 0) < 0) {
- goto cleanup;
+ return -1;
}
if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
@@ -552,15 +545,14 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
goto malformed_resp;
- if (-err->error == EOPNOTSUPP && fallback) {
- rc = fallback(ifname);
- goto cleanup;
- }
+ if (-err->error == EOPNOTSUPP && fallback)
+ return fallback(ifname);
+
if (err->error) {
virReportSystemError(-err->error,
_("error destroying network device %s"),
ifname);
- goto cleanup;
+ return -1;
}
break;
@@ -571,20 +563,17 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
goto malformed_resp;
}
- rc = 0;
- cleanup:
- nlmsg_free(nl_msg);
- return rc;
+ return 0;
malformed_resp:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- goto cleanup;
+ return -1;
buffer_too_small:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- goto cleanup;
+ return -1;
}
/**
@@ -605,13 +594,12 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
int
virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
{
- int rc = -1;
struct nlmsgerr *err;
struct ndmsg ndinfo = {
.ndm_family = AF_UNSPEC,
};
unsigned int recvbuflen;
- struct nl_msg *nl_msg;
+ VIR_AUTOPTR(virNlMsg) nl_msg = NULL;
VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
*nlData = NULL;
@@ -628,7 +616,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
- goto cleanup;
+ return -1;
if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
goto malformed_resp;
@@ -642,7 +630,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
if (err->error) {
virReportSystemError(-err->error,
"%s", _("error dumping"));
- goto cleanup;
+ return -1;
}
break;
@@ -654,21 +642,17 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
}
VIR_STEAL_PTR(*nlData, resp);
- rc = recvbuflen;
-
- cleanup:
- nlmsg_free(nl_msg);
- return rc;
+ return recvbuflen;
malformed_resp:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- goto cleanup;
+ return -1;
buffer_too_small:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- goto cleanup;
+ return -1;
}
int
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Aug 09, 2018 at 09:42:12AM +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@gmail.com> > --- > src/util/virnetlink.c | 72 ++++++++++++++++++++------------------------------- > 1 file changed, 28 insertions(+), 44 deletions(-) > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index fcdc09d..66e80e2 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -297,15 +297,16 @@ int virNetlinkCommand(struct nl_msg *nl_msg, > uint32_t src_pid, uint32_t dst_pid, > unsigned int protocol, unsigned int groups) > { > - int ret = -1; > struct sockaddr_nl nladdr = { > .nl_family = AF_NETLINK, > .nl_pid = dst_pid, > .nl_groups = 0, > }; > struct pollfd fds[1]; > - VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; > int len = 0; > + VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; unjustified code movement... > + > + *respbuflen = 0; unnecessary initialization.. > > memset(fds, 0, sizeof(fds)); > > @@ -324,15 +325,12 @@ int virNetlinkCommand(struct nl_msg *nl_msg, > goto cleanup; > } > > - ret = 0; > *respbuflen = len; > - cleanup: > - if (ret < 0) { > - *resp = NULL; > - *respbuflen = 0; > - } > + return 0; > > - return ret; > + cleanup: > + *resp = NULL; > + return -1; I moved ^this hunk into the previous patch as I converted 1 more var into VIR_AUTOFREE. Reviewed-by: Erik Skultety <eskultet@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 13 Aug 2018 at 18:10, Erik Skultety <eskultet@redhat.com> wrote: > > On Thu, Aug 09, 2018 at 09:42:12AM +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@gmail.com> > > --- > > src/util/virnetlink.c | 72 ++++++++++++++++++++------------------------------- > > 1 file changed, 28 insertions(+), 44 deletions(-) > > > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > > index fcdc09d..66e80e2 100644 > > --- a/src/util/virnetlink.c > > +++ b/src/util/virnetlink.c > > @@ -297,15 +297,16 @@ int virNetlinkCommand(struct nl_msg *nl_msg, > > uint32_t src_pid, uint32_t dst_pid, > > unsigned int protocol, unsigned int groups) > > { > > - int ret = -1; > > struct sockaddr_nl nladdr = { > > .nl_family = AF_NETLINK, > > .nl_pid = dst_pid, > > .nl_groups = 0, > > }; > > struct pollfd fds[1]; > > - VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; > > int len = 0; > > + VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; > > unjustified code movement... > > > + > > + *respbuflen = 0; > > unnecessary initialization.. We need *respbuflen to be 0 if -1 is returned. So if this initialization is removed, we need to add `*respbuflen = 0;` in the cleanup section below. > > memset(fds, 0, sizeof(fds)); > > > > @@ -324,15 +325,12 @@ int virNetlinkCommand(struct nl_msg *nl_msg, > > goto cleanup; > > } > > > > - ret = 0; > > *respbuflen = len; > > - cleanup: > > - if (ret < 0) { > > - *resp = NULL; > > - *respbuflen = 0; > > - } > > + return 0; > > > > - return ret; > > + cleanup: > > + *resp = NULL; > > + return -1; > > I moved ^this hunk into the previous patch as I converted 1 more var into > VIR_AUTOFREE. > > Reviewed-by: Erik Skultety <eskultet@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 13, 2018 at 10:23:02PM +0530, Sukrit Bhatnagar wrote: > On Mon, 13 Aug 2018 at 18:10, Erik Skultety <eskultet@redhat.com> wrote: > > > > On Thu, Aug 09, 2018 at 09:42:12AM +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@gmail.com> > > > --- > > > src/util/virnetlink.c | 72 ++++++++++++++++++++------------------------------- > > > 1 file changed, 28 insertions(+), 44 deletions(-) > > > > > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > > > index fcdc09d..66e80e2 100644 > > > --- a/src/util/virnetlink.c > > > +++ b/src/util/virnetlink.c > > > @@ -297,15 +297,16 @@ int virNetlinkCommand(struct nl_msg *nl_msg, > > > uint32_t src_pid, uint32_t dst_pid, > > > unsigned int protocol, unsigned int groups) > > > { > > > - int ret = -1; > > > struct sockaddr_nl nladdr = { > > > .nl_family = AF_NETLINK, > > > .nl_pid = dst_pid, > > > .nl_groups = 0, > > > }; > > > struct pollfd fds[1]; > > > - VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; > > > int len = 0; > > > + VIR_AUTOPTR(virNetlinkHandle) nlhandle = NULL; > > > > unjustified code movement... > > > > > + > > > + *respbuflen = 0; > > > > unnecessary initialization.. > > We need *respbuflen to be 0 if -1 is returned. So if this initialization is > removed, we need to add `*respbuflen = 0;` in the cleanup section below. Why exactly do we need it? If -1 is to be returned, the caller should not be touching the pointers afterwards, if they are, it's a bug in the caller. Quick grepping on usage of virNetlinkCommand didn't show anything suspicious - we always either return immediately or jump to cleanup for that matter. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.