Re: [libvirt] [PATCH 4/6] util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE

11 Sep 2018

On Mon, Sep 10, 2018 at 11:47:53AM +0800, Shi Lei wrote:
...
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro,
many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
 src/util/virnetdev.c | 253 +++++++++++++++----------------------------
 1 file changed, 87 insertions(+), 166 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5d4ad24..7f43f15 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -200,27 +200,22 @@ virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED,
  */
 int virNetDevExists(const char *ifname)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
if (ioctl(fd, SIOCGIFFLAGS, &ifr)) {
         if (errno == ENODEV || errno == ENXIO)
-            ret = 0;
-        else
-            virReportSystemError(errno,
-                                 _("Unable to check interface flags for %s"), ifname);
-        goto cleanup;
-    }
+            return 0;
-    ret = 1;
+        virReportSystemError(errno, _("Unable to check interface flags for %s"),
+                             ifname);
+        return -1;
+    }
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 1;
 }
 #else
 int virNetDevExists(const char *ifname)
@@ -251,20 +246,20 @@ virNetDevSetMACInternal(const char *ifname,
                         const virMacAddr *macaddr,
                         bool quiet)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
     char macstr[VIR_MAC_STRING_BUFLEN];
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
/* To fill ifr.ifr_hdaddr.sa_family field */
     if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) {
-        virReportSystemError(errno,
-                             _("Cannot get interface MAC on '%s'"),
+        virReportSystemError(errno, _("Cannot get interface MAC on '%s'"),
                              ifname);
-        goto cleanup;
+
+        VIR_DEBUG("SIOCSIFHWADDR %s get MAC - Fail", ifname);
+        return -1;
     }
virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
@@ -272,24 +267,22 @@ virNetDevSetMACInternal(const char *ifname,
     if (ioctl(fd, SIOCSIFHWADDR, &ifr) < 0) {
if (quiet &&
-            (errno == EADDRNOTAVAIL || errno == EPERM))
-            goto cleanup;
+            (errno == EADDRNOTAVAIL || errno == EPERM)) {
+            VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Fail",
+                      ifname, virMacAddrFormat(macaddr, macstr));
+            return -1;
+        }
virReportSystemError(errno,
                              _("Cannot set interface MAC to %s on '%s'"),
                              virMacAddrFormat(macaddr, macstr), ifname);
-        goto cleanup;
+        return -1;
     }
-    ret = 0;
+    VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Success",
+              ifname, virMacAddrFormat(macaddr, macstr));
- cleanup:
-    VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - %s",
-              ifname, virMacAddrFormat(macaddr, macstr),
-              ret < 0 ? "Fail" : "Success");
-
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
@@ -305,8 +298,7 @@ virNetDevSetMACInternal(const char *ifname,
         struct ifreq ifr;
         struct sockaddr_dl sdl;
         char mac[VIR_MAC_STRING_BUFLEN + 1] = ":";
-        int s;
-        int ret = -1;
+        VIR_AUTOCLOSE(s);
if ((s = virNetDevSetupControl(ifname, &ifr)) < 0)
             return -1;
@@ -320,23 +312,19 @@ virNetDevSetMACInternal(const char *ifname,
if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) {
             if (quiet &&
-                (errno == EADDRNOTAVAIL || errno == EPERM))
-                goto cleanup;
+                (errno == EADDRNOTAVAIL || errno == EPERM)) {
+                VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Fail", ifname, mac + 1);
+                return -1;
+            }
virReportSystemError(errno,
                                  _("Cannot set interface MAC to %s on '%s'"),
                                  mac + 1, ifname);
-            goto cleanup;
+            return -1;
         }
-        ret = 0;
- cleanup:
-        VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - %s", ifname, mac + 1,
-                  ret < 0 ? "Fail" : "Success");
-
-        VIR_FORCE_CLOSE(s);
-
-        return ret;
+        VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Success", ifname, mac + 1);
+        return 0;
 }
@@ -379,9 +367,8 @@ virNetDevSetMAC(const char *ifname,
 int virNetDevGetMAC(const char *ifname,
                     virMacAddrPtr macaddr)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -390,16 +377,12 @@ int virNetDevGetMAC(const char *ifname,
         virReportSystemError(errno,
                              _("Cannot get interface MAC on '%s'"),
                              ifname);
-        goto cleanup;
+        return -1;
     }
virMacAddrSetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
-    ret = 0;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
 #else
 int virNetDevGetMAC(const char *ifname,
@@ -424,9 +407,8 @@ int virNetDevGetMAC(const char *ifname,
  */
 int virNetDevGetMTU(const char *ifname)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -435,14 +417,10 @@ int virNetDevGetMTU(const char *ifname)
         virReportSystemError(errno,
                              _("Cannot get interface MTU on '%s'"),
                              ifname);
-        goto cleanup;
+        return -1;
     }
-    ret = ifr.ifr_mtu;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return ifr.ifr_mtu;
 }
 #else
 int virNetDevGetMTU(const char *ifname)
@@ -468,9 +446,8 @@ int virNetDevGetMTU(const char *ifname)
  */
 int virNetDevSetMTU(const char *ifname, int mtu)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -481,14 +458,10 @@ int virNetDevSetMTU(const char *ifname, int mtu)
         virReportSystemError(errno,
                              _("Cannot set interface MTU on '%s'"),
                              ifname);
-        goto cleanup;
+        return -1;
     }
-    ret = 0;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
 #else
 int virNetDevSetMTU(const char *ifname, int mtu ATTRIBUTE_UNUSED)
@@ -592,9 +565,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
  */
 int virNetDevSetName(const char* ifname, const char *newifname)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -604,7 +576,7 @@ int virNetDevSetName(const char* ifname, const char *newifname)
         virReportSystemError(ERANGE,
                              _("Network interface name '%s' is too long"),
                              newifname);
-        goto cleanup;
+        return -1;
     }
 # else
     ifr.ifr_data = (caddr_t)newifname;
@@ -614,14 +586,10 @@ int virNetDevSetName(const char* ifname, const char *newifname)
         virReportSystemError(errno,
                              _("Unable to rename '%s' to '%s'"),
                              ifname, newifname);
-        goto cleanup;
+        return -1;
     }
-    ret = 0;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
 #else
 int virNetDevSetName(const char* ifname, const char *newifname)
@@ -638,10 +606,9 @@ int virNetDevSetName(const char* ifname, const char *newifname)
 static int
 virNetDevSetIFFlag(const char *ifname, int flag, bool val)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
     int ifflags;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -650,7 +617,7 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val)
         virReportSystemError(errno,
                              _("Cannot get interface flags on '%s'"),
                              ifname);
-        goto cleanup;
+        return -1;
     }
if (val)
@@ -664,15 +631,11 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val)
             virReportSystemError(errno,
                                  _("Cannot set interface flags on '%s'"),
                                  ifname);
-            goto cleanup;
+            return -1;
         }
     }
-    ret = 0;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
 #else
 static int
@@ -765,9 +728,8 @@ virNetDevSetRcvAllMulti(const char *ifname,
 static int
 virNetDevGetIFFlag(const char *ifname, int flag, bool *val)
 {
-    int fd = -1;
-    int ret = -1;
     struct ifreq ifr;
+    VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
         return -1;
@@ -776,15 +738,11 @@ virNetDevGetIFFlag(const char *ifname, int flag, bool *val)
         virReportSystemError(errno,
                              _("Cannot get interface flags on '%s'"),
                              ifname);
-        goto cleanup;
+        return -1;
     }
*val = (ifr.ifr_flags & flag) ? true : false;
-    ret = 0;
-
- cleanup:
-    VIR_FORCE_CLOSE(fd);
-    return ret;
+    return 0;
 }
 #else
 static int
@@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex)
 #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ)
 int virNetDevGetIndex(const char *ifname, int *ifindex)
 {
-    int ret = -1;
     struct ifreq ifreq;
-    int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
+    VIR_AUTOCLOSE(fd);
+    socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
^this could not potentially work...

Erik

    

Re: [libvirt] [PATCH 4/6] util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE

Erik Skultety