On Thu, Aug 09, 2018 at 09:42:17AM +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/virnetdevip.c | 130 +++++++++++++++++++------------------------------ 1 file changed, 51 insertions(+), 79 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index c6d6175..4a38a54 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -168,10 +168,9 @@ virNetDevIPAddrAdd(const char *ifname, virSocketAddr *peer, unsigned int prefix) { - virSocketAddr *broadcast = NULL; - int ret = -1; - struct nl_msg *nlmsg = NULL; unsigned int recvbuflen; + VIR_AUTOPTR(virNlMsg) nlmsg = NULL; + VIR_AUTOPTR(virSocketAddr) broadcast = NULL; VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; VIR_AUTOFREE(char *) ipStr = NULL; VIR_AUTOFREE(char *) peerStr = NULL; @@ -186,13 +185,13 @@ virNetDevIPAddrAdd(const char *ifname, !(peer && VIR_SOCKET_ADDR_VALID(peer))) { /* compute a broadcast address if this is IPv4 */ if (VIR_ALLOC(broadcast) < 0) - goto cleanup; + return -1;
if (virSocketAddrBroadcastByPrefix(addr, prefix, broadcast) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to determine broadcast address for '%s/%d'"), - ipStr, prefix); - goto cleanup; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to determine broadcast address for '%s/%d'"), + ipStr, prefix); + return -1; } bcastStr = virSocketAddrFormat(broadcast); } @@ -206,11 +205,11 @@ virNetDevIPAddrAdd(const char *ifname, if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_NEWADDR, ifname, addr, prefix, broadcast, peer))) - goto cleanup; + return -1;
if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) - goto cleanup; + return -1;
if (virNetlinkGetErrorCode(resp, recvbuflen) < 0) { @@ -220,14 +219,10 @@ virNetDevIPAddrAdd(const char *ifname, peerStr ? " peer " : "", peerStr ? peerStr : "", bcastStr ? " bcast " : "", bcastStr ? bcastStr : "", ifname); - goto cleanup; + return -1; }
- ret = 0; - cleanup: - nlmsg_free(nlmsg); - VIR_FREE(broadcast); - return ret; + return 0; }
@@ -246,30 +241,26 @@ virNetDevIPAddrDel(const char *ifname, virSocketAddr *addr, unsigned int prefix) { - int ret = -1; - struct nl_msg *nlmsg = NULL; unsigned int recvbuflen; + VIR_AUTOPTR(virNlMsg) nlmsg = NULL; VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_DELADDR, ifname, addr, prefix, NULL, NULL))) - goto cleanup; + return -1;
if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) - goto cleanup; + return -1;
if (virNetlinkGetErrorCode(resp, recvbuflen) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, _("Error removing IP address from %s"), ifname); - goto cleanup; + return -1; }
- ret = 0; - cleanup: - nlmsg_free(nlmsg); - return ret; + return 0; }
@@ -292,8 +283,6 @@ virNetDevIPRouteAdd(const char *ifname, virSocketAddrPtr gateway, unsigned int metric) { - int ret = -1; - struct nl_msg *nlmsg = NULL; struct nlmsghdr *resp = NULL; unsigned int recvbuflen; unsigned int ifindex; @@ -304,6 +293,7 @@ virNetDevIPRouteAdd(const char *ifname, int errCode; virSocketAddr defaultAddr; virSocketAddrPtr actualAddr; + VIR_AUTOPTR(virNlMsg) nlmsg = NULL; VIR_AUTOFREE(char *) toStr = NULL; VIR_AUTOFREE(char *) viaStr = NULL;
@@ -315,10 +305,10 @@ virNetDevIPRouteAdd(const char *ifname, int family = VIR_SOCKET_ADDR_FAMILY(gateway); if (family == AF_INET) { if (virSocketAddrParseIPv4(&defaultAddr, VIR_SOCKET_ADDR_IPV4_ALL) < 0) - goto cleanup; + return -1; } else { if (virSocketAddrParseIPv6(&defaultAddr, VIR_SOCKET_ADDR_IPV6_ALL) < 0) - goto cleanup; + return -1; }
actualAddr = &defaultAddr; @@ -330,17 +320,17 @@ virNetDevIPRouteAdd(const char *ifname,
if (virNetDevGetIPAddressBinary(actualAddr, &addrData, &addrDataLen) < 0 || virNetDevGetIPAddressBinary(gateway, &gatewayData, &addrDataLen) < 0) - goto cleanup; + return -1;
/* Get the interface index */ if ((ifindex = if_nametoindex(ifname)) == 0) - goto cleanup; + return -1;
if (!(nlmsg = nlmsg_alloc_simple(RTM_NEWROUTE, NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL))) { virReportOOMError(); - goto cleanup; + return -1; }
memset(&rtmsg, 0, sizeof(rtmsg)); @@ -369,22 +359,19 @@ virNetDevIPRouteAdd(const char *ifname,
if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) - goto cleanup; + return -1;
if ((errCode = virNetlinkGetErrorCode(resp, recvbuflen)) < 0) { virReportSystemError(errCode, _("Error adding route to %s"), ifname); - goto cleanup; + return -1; }
- ret = 0; - cleanup: - nlmsg_free(nlmsg); - return ret; + return 0;
buffer_too_small: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("allocated netlink buffer is too small")); - goto cleanup; + return -1; }
@@ -443,12 +430,11 @@ virNetDevIPParseDadStatus(struct nlmsghdr *nlh, int len, int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) { - struct nl_msg *nlmsg = NULL; struct ifaddrmsg ifa; unsigned int recvbuflen; - int ret = -1; bool dad = true; time_t max_time = time(NULL) + VIR_DAD_WAIT_TIMEOUT; + VIR_AUTOPTR(virNlMsg) nlmsg = NULL;
if (!(nlmsg = nlmsg_alloc_simple(RTM_GETADDR, NLM_F_REQUEST | NLM_F_DUMP))) { @@ -462,7 +448,7 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) if (nlmsg_append(nlmsg, &ifa, sizeof(ifa), NLMSG_ALIGNTO) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("allocated netlink buffer is too small")); - goto cleanup; + return -1; }
/* Periodically query netlink until DAD finishes on all known addresses. */ @@ -471,12 +457,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) - goto cleanup; + return -1;
if (virNetlinkGetErrorCode(resp, recvbuflen) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("error reading DAD state information")); - goto cleanup; + return -1; }
/* Parse response. */ @@ -490,21 +476,19 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) _("Duplicate Address Detection " "not finished in %d seconds"), VIR_DAD_WAIT_TIMEOUT); } else { - ret = 0; + return 0; }
- cleanup: - nlmsg_free(nlmsg); - return ret; + return -1; }
static int virNetDevIPGetAcceptRA(const char *ifname) { + char *suffix; + int accept_ra = -1; VIR_AUTOFREE(char *) path = NULL; VIR_AUTOFREE(char *) buf = NULL; - char *suffix; - int accept_ra = -1;
So what's the purpose of ^this hunk? I know you'd like the macros to be ordered at the very end of the declare block, however, that's not enough of a justification for code movement, remember, it's nice to have, but not absolutely required. I'll drop it locally.
if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra", ifname ? ifname : "all") < 0) @@ -553,6 +537,7 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, /* Should never happen: netlink message would be broken */ if (ifname) { VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif); +
Same ^here, I'll drop this hunk. Apart from that: Reviewed-by: Erik Skultety <eskultet@redhat.com>