[libvirt] [PATCH] util: close the ioctl socket at the end of if(Get|Set)MacAddress
Otherwise this will leak an fd each time one of these functions is called. --- src/util/interface.c | 36 ++++++++++++++++++++++++++---------- 1 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/util/interface.c b/src/util/interface.c index f486124..178a4dd 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -413,6 +413,7 @@ ifaceGetMacAddress(const char *ifname, { struct ifreq ifr; int fd; + int rc = 0; if (!ifname) return EINVAL; @@ -422,15 +423,21 @@ ifaceGetMacAddress(const char *ifname, return errno; memset(&ifr, 0, sizeof(struct ifreq)); - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + rc = EINVAL; + goto err_exit; + } - if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) - return errno; + if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) { + rc = errno; + goto err_exit; + } memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN); - return 0; +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; } #else @@ -461,6 +468,7 @@ ifaceSetMacAddress(const char *ifname, { struct ifreq ifr; int fd; + int rc = 0; if (!ifname) return EINVAL; @@ -470,16 +478,24 @@ ifaceSetMacAddress(const char *ifname, return errno; memset(&ifr, 0, sizeof(struct ifreq)); - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + rc = EINVAL; + goto err_exit; + } /* To fill ifr.ifr_hdaddr.sa_family field */ - if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) - return errno; + if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) { + rc = errno; + goto err_exit; + } memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN); - return ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno; + rc = ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno; + +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; } #else -- 1.7.3.4
2011/7/6 Laine Stump <laine@laine.org>:
Otherwise this will leak an fd each time one of these functions is called. --- src/util/interface.c | 36 ++++++++++++++++++++++++++---------- 1 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c index f486124..178a4dd 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -413,6 +413,7 @@ ifaceGetMacAddress(const char *ifname, { struct ifreq ifr; int fd; + int rc = 0;
if (!ifname) return EINVAL; @@ -422,15 +423,21 @@ ifaceGetMacAddress(const char *ifname, return errno;
memset(&ifr, 0, sizeof(struct ifreq)); - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + rc = EINVAL; + goto err_exit; + }
- if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) - return errno; + if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) { + rc = errno; + goto err_exit; + }
memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN);
- return 0; +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; }
#else @@ -461,6 +468,7 @@ ifaceSetMacAddress(const char *ifname, { struct ifreq ifr; int fd; + int rc = 0;
if (!ifname) return EINVAL; @@ -470,16 +478,24 @@ ifaceSetMacAddress(const char *ifname, return errno;
memset(&ifr, 0, sizeof(struct ifreq)); - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { + rc = EINVAL; + goto err_exit; + }
/* To fill ifr.ifr_hdaddr.sa_family field */ - if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) - return errno; + if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) { + rc = errno; + goto err_exit; + }
memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN);
- return ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno; + rc = ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno; + +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; }
HACKING suggests to call the label cleanup instead of err_exit in both functions. ACK, with the labels renamed. -- Matthias Bolte http://photron.blogspot.com
On 07/06/2011 07:31 AM, Matthias Bolte wrote:
2011/7/6 Laine Stump<laine@laine.org>:
Otherwise this will leak an fd each time one of these functions is called. --- src/util/interface.c | 36 ++++++++++++++++++++++++++---------- 1 files changed, 26 insertions(+), 10 deletions(-) + +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; } HACKING suggests to call the label cleanup instead of err_exit in both functions.
Yeah, but all the other functions in that file had err_exit, and I wanted to be consistent within the file, but didn't want to rename everything.
ACK, with the labels renamed.
Only if you'll ACK a separate patch that renames the other labels :-)
2011/7/6 Laine Stump <laine@laine.org>:
On 07/06/2011 07:31 AM, Matthias Bolte wrote:
2011/7/6 Laine Stump<laine@laine.org>:
Otherwise this will leak an fd each time one of these functions is called. --- src/util/interface.c | 36 ++++++++++++++++++++++++++---------- 1 files changed, 26 insertions(+), 10 deletions(-) + +err_exit: + VIR_FORCE_CLOSE(fd); + return rc; }
HACKING suggests to call the label cleanup instead of err_exit in both functions.
Yeah, but all the other functions in that file had err_exit, and I wanted to be consistent within the file, but didn't want to rename everything.
ACK, with the labels renamed.
Only if you'll ACK a separate patch that renames the other labels :-)
Oh, I didn't notice that. All err_exit in there qualify for being renamed to cleanup, so there's no reason why I wouldn't ACK such a patch :) -- Matthias Bolte http://photron.blogspot.com
This brings it in line with the recommendations in HACKING. --- src/util/interface.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/util/interface.c b/src/util/interface.c index 32cd4eb..4037efb 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -156,7 +156,7 @@ static int chgIfaceFlags(const char *ifname, short flagclear, short flagset) { rc = getFlags(fd, ifname, &ifr); if (rc != 0) - goto err_exit; + goto cleanup; flags = (ifr.ifr_flags & flagmask) | flagset; @@ -167,7 +167,7 @@ static int chgIfaceFlags(const char *ifname, short flagclear, short flagset) { rc = errno; } -err_exit: +cleanup: VIR_FORCE_CLOSE(fd); return rc; } @@ -241,7 +241,7 @@ ifaceCheck(bool reportError, const char *ifname, _("invalid interface name %s"), ifname); rc = EINVAL; - goto err_exit; + goto cleanup; } if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) { @@ -250,12 +250,12 @@ ifaceCheck(bool reportError, const char *ifname, _("coud not get MAC address of interface %s"), ifname); rc = errno; - goto err_exit; + goto cleanup; } if (memcmp(&ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN) != 0) { rc = ENODEV; - goto err_exit; + goto cleanup; } } @@ -265,7 +265,7 @@ ifaceCheck(bool reportError, const char *ifname, rc = ENODEV; } - err_exit: + cleanup: VIR_FORCE_CLOSE(fd); return rc; @@ -318,7 +318,7 @@ ifaceGetIndex(bool reportError, const char *ifname, int *ifindex) _("invalid interface name %s"), ifname); rc = EINVAL; - goto err_exit; + goto cleanup; } if (ioctl(fd, SIOCGIFINDEX, &ifreq) >= 0) @@ -331,7 +331,7 @@ ifaceGetIndex(bool reportError, const char *ifname, int *ifindex) rc = ENODEV; } -err_exit: +cleanup: VIR_FORCE_CLOSE(fd); return rc; @@ -368,17 +368,17 @@ ifaceGetVlanID(const char *vlanifname, int *vlanid) { if (virStrcpyStatic(vlanargs.device1, vlanifname) == NULL) { rc = EINVAL; - goto err_exit; + goto cleanup; } if (ioctl(fd, SIOCGIFVLAN, &vlanargs) != 0) { rc = errno; - goto err_exit; + goto cleanup; } *vlanid = vlanargs.u.VID; - err_exit: + cleanup: VIR_FORCE_CLOSE(fd); return rc; @@ -636,7 +636,7 @@ ifaceMacvtapLinkAdd(const char *type, if (nlComm(nl_msg, &recvbuf, &recvbuflen, 0) < 0) { rc = -1; - goto err_exit; + goto cleanup; } if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) @@ -675,7 +675,7 @@ ifaceMacvtapLinkAdd(const char *type, goto malformed_resp; } -err_exit: +cleanup: nlmsg_free(nl_msg); VIR_FREE(recvbuf); @@ -760,7 +760,7 @@ ifaceLinkDel(const char *ifname) if (nlComm(nl_msg, &recvbuf, &recvbuflen, 0) < 0) { rc = -1; - goto err_exit; + goto cleanup; } if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) @@ -789,7 +789,7 @@ ifaceLinkDel(const char *ifname) goto malformed_resp; } -err_exit: +cleanup: nlmsg_free(nl_msg); VIR_FREE(recvbuf); @@ -892,13 +892,13 @@ ifaceMacvtapLinkDump(bool nltarget_kernel, const char *ifname, int ifindex, pid = getPidFunc(); if (pid == 0) { rc = -1; - goto err_exit; + goto cleanup; } } if (nlComm(nl_msg, recvbuf, &recvbuflen, pid) < 0) { rc = -1; - goto err_exit; + goto cleanup; } if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL) @@ -935,7 +935,7 @@ ifaceMacvtapLinkDump(bool nltarget_kernel, const char *ifname, int ifindex, if (rc != 0) VIR_FREE(*recvbuf); -err_exit: +cleanup: nlmsg_free(nl_msg); return rc; -- 1.7.3.4
2011/7/6 Laine Stump <laine@laine.org>:
This brings it in line with the recommendations in HACKING. --- src/util/interface.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
participants (2)
-
Laine Stump -
Matthias Bolte