util: improve virNetDevTapGetRealDeviceName

[libvirt] [PATCH] util: improve virNetDevTapGetRealDeviceName

Posted by Roman Bogorodskiy 6 years ago

virNetDevTapGetRealDeviceName() is used on FreeBSD because interface
names (such as one sees in output of tools like ifconfig(8)) might not
match their /dev entity names, and for bhyve we need the latter.

Current implementation is not very efficient because in order to find
/dev name, it goes through all /dev/tap* entries and tries to issue
TAPGIFNAME ioctl on it. Not only this is slow, but also there's a bug in
this implementation when more than one NIC is passed to a VM: once we
find the tap interface we're looking for, we set its state to UP because
opening it for issuing ioctl sets it DOWN, even if it was UP before.
When we have more than 1 NIC for a VM, we have only last one UP because
others remain DOWN after unsuccessful attempts to match interface name.

New implementation just uses sysctl(3), so it should be faster and
won't make interfaces go down to get name.
---
 src/util/virnetdevtap.c | 71 +++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 41 deletions(-)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index a3ed59da8..afe4f0b3c 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -48,7 +48,8 @@
 #ifdef __linux__
 # include <linux/if_tun.h>    /* IFF_TUN, IFF_NO_PI */
 #elif defined(__FreeBSD__)
-# include <net/if_tap.h>
+# include <net/if_mib.h>
+# include <sys/sysctl.h>
 #endif
 #if defined(HAVE_GETIFADDRS) && defined(AF_LINK)
 # include <ifaddrs.h>
@@ -101,55 +102,43 @@ virNetDevTapGetName(int tapfd ATTRIBUTE_UNUSED, char **ifname ATTRIBUTE_UNUSED)
 char*
 virNetDevTapGetRealDeviceName(char *ifname ATTRIBUTE_UNUSED)
 {
-#ifdef TAPGIFNAME
+#ifdef IFDATA_DRIVERNAME
+    int ifindex = 0;
+    int name[6];
+    size_t len = 0;
     char *ret = NULL;
-    struct dirent *dp;
-    DIR *dirp = NULL;
-    char *devpath = NULL;
-    int fd;
 
-    if (virDirOpen(&dirp, "/dev") < 0)
+    if ((ifindex = if_nametoindex(ifname)) == 0) {
+        virReportSystemError(errno,
+                             _("Unable to get interface index for '%s'"),
+                             ifname);
         return NULL;
+    }
 
-    while (virDirRead(dirp, &dp, "/dev") > 0) {
-        if (STRPREFIX(dp->d_name, "tap")) {
-            struct ifreq ifr;
-            if (virAsprintf(&devpath, "/dev/%s", dp->d_name) < 0)
-                goto cleanup;
-            if ((fd = open(devpath, O_RDWR)) < 0) {
-                if (errno == EBUSY) {
-                    VIR_FREE(devpath);
-                    continue;
-                }
+    name[0] = CTL_NET;
+    name[1] = PF_LINK;
+    name[2] = NETLINK_GENERIC;
+    name[3] = IFMIB_IFDATA;
+    name[4] = ifindex;
+    name[5] = IFDATA_DRIVERNAME;
 
-                virReportSystemError(errno, _("Unable to open '%s'"), devpath);
-                goto cleanup;
-            }
-
-            if (ioctl(fd, TAPGIFNAME, (void *)&ifr) < 0) {
-                virReportSystemError(errno, "%s",
-                                     _("Unable to query tap interface name"));
-                goto cleanup;
-            }
+    if (sysctl(name, 6, NULL, &len, 0, 0) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to get driver name for '%s'"),
+                             ifname);
+        return NULL;
+    }
 
-            if (STREQ(ifname, ifr.ifr_name)) {
-                /* we can ignore the return value
-                 * because we still have nothing
-                 * to do but return;
-                 */
-                ignore_value(VIR_STRDUP(ret, dp->d_name));
-                goto cleanup;
-            }
+    ret = malloc(len);
 
-            VIR_FREE(devpath);
-            VIR_FORCE_CLOSE(fd);
-        }
+    if (sysctl(name, 6, ret, &len, 0, 0) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to get driver name for '%s'"),
+                             ifname);
+        free(ret);
+        return NULL;
     }
 
- cleanup:
-    VIR_FREE(devpath);
-    VIR_FORCE_CLOSE(fd);
-    VIR_DIR_CLOSE(dirp);
     return ret;
 #else
     return NULL;
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: improve virNetDevTapGetRealDeviceName

Posted by Michal Privoznik 6 years ago

On 04/08/2018 05:40 PM, Roman Bogorodskiy wrote:
> virNetDevTapGetRealDeviceName() is used on FreeBSD because interface
> names (such as one sees in output of tools like ifconfig(8)) might not
> match their /dev entity names, and for bhyve we need the latter.
> 
> Current implementation is not very efficient because in order to find
> /dev name, it goes through all /dev/tap* entries and tries to issue
> TAPGIFNAME ioctl on it. Not only this is slow, but also there's a bug in
> this implementation when more than one NIC is passed to a VM: once we
> find the tap interface we're looking for, we set its state to UP because
> opening it for issuing ioctl sets it DOWN, even if it was UP before.
> When we have more than 1 NIC for a VM, we have only last one UP because
> others remain DOWN after unsuccessful attempts to match interface name.
> 
> New implementation just uses sysctl(3), so it should be faster and
> won't make interfaces go down to get name.
> ---
>  src/util/virnetdevtap.c | 71 +++++++++++++++++++++----------------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)

ACK with this squashed in:

diff --git i/src/util/virnetdevtap.c w/src/util/virnetdevtap.c
index afe4f0b3c1..bd0710ad2e 100644
--- i/src/util/virnetdevtap.c
+++ w/src/util/virnetdevtap.c
@@ -40,7 +40,6 @@
 #include <string.h>
 #include <unistd.h>
 #include <regex.h>
-#include <dirent.h>
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <net/if.h>
@@ -129,13 +128,14 @@ virNetDevTapGetRealDeviceName(char *ifname ATTRIBUTE_UNUSED)
         return NULL;
     }
 
-    ret = malloc(len);
+    if (VIR_ALLOC_N(ret, len) < 0)
+        return NULL;
 
     if (sysctl(name, 6, ret, &len, 0, 0) < 0) {
         virReportSystemError(errno,
                              _("Unable to get driver name for '%s'"),
                              ifname);
-        free(ret);
+        VIR_FREE(ret);
         return NULL;
     }
 


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: improve virNetDevTapGetRealDeviceName

Posted by Roman Bogorodskiy 5 years, 12 months ago

  Michal Privoznik wrote:

> On 04/08/2018 05:40 PM, Roman Bogorodskiy wrote:
> > virNetDevTapGetRealDeviceName() is used on FreeBSD because interface
> > names (such as one sees in output of tools like ifconfig(8)) might not
> > match their /dev entity names, and for bhyve we need the latter.
> > 
> > Current implementation is not very efficient because in order to find
> > /dev name, it goes through all /dev/tap* entries and tries to issue
> > TAPGIFNAME ioctl on it. Not only this is slow, but also there's a bug in
> > this implementation when more than one NIC is passed to a VM: once we
> > find the tap interface we're looking for, we set its state to UP because
> > opening it for issuing ioctl sets it DOWN, even if it was UP before.
> > When we have more than 1 NIC for a VM, we have only last one UP because
> > others remain DOWN after unsuccessful attempts to match interface name.
> > 
> > New implementation just uses sysctl(3), so it should be faster and
> > won't make interfaces go down to get name.
> > ---
> >  src/util/virnetdevtap.c | 71 +++++++++++++++++++++----------------------------
> >  1 file changed, 30 insertions(+), 41 deletions(-)
> 
> ACK with this squashed in:
> 
> diff --git i/src/util/virnetdevtap.c w/src/util/virnetdevtap.c
> index afe4f0b3c1..bd0710ad2e 100644
> --- i/src/util/virnetdevtap.c
> +++ w/src/util/virnetdevtap.c
> @@ -40,7 +40,6 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <regex.h>
> -#include <dirent.h>
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <net/if.h>
> @@ -129,13 +128,14 @@ virNetDevTapGetRealDeviceName(char *ifname ATTRIBUTE_UNUSED)
>          return NULL;
>      }
>  
> -    ret = malloc(len);
> +    if (VIR_ALLOC_N(ret, len) < 0)
> +        return NULL;
>  
>      if (sysctl(name, 6, ret, &len, 0, 0) < 0) {
>          virReportSystemError(errno,
>                               _("Unable to get driver name for '%s'"),
>                               ifname);
> -        free(ret);
> +        VIR_FREE(ret);
>          return NULL;
>      }
>  
> 
> 
> Michal

Pushed, thanks!

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list