Patchwork [try,2,version,2] qemu/tap: add -net tap, dev= option

login
register
mail settings
Submitter Arnd Bergmann
Date Dec. 9, 2009, 2:49 p.m.
Message ID <200912091549.04872.arnd@arndb.de>
Download mbox | patch
Permalink /patch/40725/
State New
Headers show

Comments

Arnd Bergmann - Dec. 9, 2009, 2:49 p.m.
In order to support macvtap, we need a way to select the actual
tap endpoint. While it would be nice to pass it by network interface
name, passing the character device is more flexible, and we can
easily do both in the long run.

This version makes it possible to use macvtap without introducing
any macvtap specific code in qemu, only a natural extension to
the existing interface.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Mark McLoughlin <markmc@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
Hopefully addressed all comments from Michael. I did compile-test
the non-linux files I touched (the solaris file failed to build for another
reason), and this now prevents passing dev= on non-linux machines.

 net.c             |    5 +++++
 net/tap-aix.c     |    3 ++-
 net/tap-bsd.c     |    9 ++++++++-
 net/tap-linux.c   |   12 ++++++++----
 net/tap-solaris.c |    7 ++++++-
 net/tap.c         |   17 ++++++++++++++---
 net/tap.h         |    3 ++-
 qemu-options.hx   |   10 +++++++++-
 8 files changed, 54 insertions(+), 12 deletions(-)
Michael S. Tsirkin - Dec. 9, 2009, 3:27 p.m.
On Wed, Dec 09, 2009 at 03:49:04PM +0100, Arnd Bergmann wrote:
> In order to support macvtap, we need a way to select the actual
> tap endpoint. While it would be nice to pass it by network interface
> name, passing the character device is more flexible, and we can
> easily do both in the long run.
> 
> This version makes it possible to use macvtap without introducing
> any macvtap specific code in qemu, only a natural extension to
> the existing interface.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Mark McLoughlin <markmc@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> ---
> Hopefully addressed all comments from Michael. I did compile-test
> the non-linux files I touched (the solaris file failed to build for another
> reason), and this now prevents passing dev= on non-linux machines.

Found a couple more minor nits in the new versions, otherwise looks good.

>  net.c             |    5 +++++
>  net/tap-aix.c     |    3 ++-
>  net/tap-bsd.c     |    9 ++++++++-
>  net/tap-linux.c   |   12 ++++++++----
>  net/tap-solaris.c |    7 ++++++-
>  net/tap.c         |   17 ++++++++++++++---
>  net/tap.h         |    3 ++-
>  qemu-options.hx   |   10 +++++++++-
>  8 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/net.c b/net.c
> index 13bdbb2..deb12fd 100644
> --- a/net.c
> +++ b/net.c
> @@ -955,6 +955,11 @@ static struct {
>              },
>  #ifndef _WIN32
>              {
> +                .name = "dev",
> +                .type = QEMU_OPT_STRING,
> +                .help = "character device pathname",
> +            },
> +            {
>                  .name = "fd",
>                  .type = QEMU_OPT_STRING,
>                  .help = "file descriptor of an already opened tap",
> diff --git a/net/tap-aix.c b/net/tap-aix.c
> index 4bc9f2f..238c190 100644
> --- a/net/tap-aix.c
> +++ b/net/tap-aix.c
> @@ -25,7 +25,8 @@
>  #include "net/tap.h"
>  #include <stdio.h>
>  
> -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> +int tap_open(char *ifname, int ifname_size, const char *dev,
> +			int *vnet_hdr, int vnet_hdr_required)
>  {
>      fprintf(stderr, "no tap on AIX\n");
>      return -1;
> diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> index 815997d..8a7334c 100644
> --- a/net/tap-bsd.c
> +++ b/net/tap-bsd.c
> @@ -40,7 +40,8 @@
>  #include <util.h>
>  #endif
>  
> -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> +int tap_open(char *ifname, int ifname_size, const char *devarg,
> +			int *vnet_hdr, int vnet_hdr_required)
>  {
>      int fd;
>      char *dev;
> @@ -80,6 +81,12 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
>      }
>  #endif
>  
> +    if (devarg) {
> +        qemu_error("dev= not supported\n");
> +        close(fd);
> +        return -1;
> +    }
> +
>      fstat(fd, &s);
>      dev = devname(s.st_rdev, S_IFCHR);
>      pstrcpy(ifname, ifname_size, dev);
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 6af9e82..d6831c0 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -32,14 +32,18 @@
>  #include "sysemu.h"
>  #include "qemu-common.h"
>  
> -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> +int tap_open(char *ifname, int ifname_size, const char *dev,
> +                        int *vnet_hdr, int vnet_hdr_required)
>  {
>      struct ifreq ifr;
>      int fd, ret;
>  
> -    TFR(fd = open("/dev/net/tun", O_RDWR));
> +    if (!*dev)
> +        dev = "/dev/net/tun";
> +

Did you test without dev parameter? I think dev will be NULL
so this will deference a nullpointer ...
probably if (!dev) is what you mean?


> +    TFR(fd = open(dev, O_RDWR));
>      if (fd < 0) {
> -        fprintf(stderr, "warning: could not open /dev/net/tun: no virtual network emulation\n");
> +        fprintf(stderr, "warning: could not open %s: no virtual network emulation\n", dev);
>          return -1;
>      }
>      memset(&ifr, 0, sizeof(ifr));
> @@ -70,7 +74,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
>          pstrcpy(ifr.ifr_name, IFNAMSIZ, "tap%d");
>      ret = ioctl(fd, TUNSETIFF, (void *) &ifr);
>      if (ret != 0) {
> -        fprintf(stderr, "warning: could not configure /dev/net/tun: no virtual network emulation\n");
> +        fprintf(stderr, "warning: could not configure %s: no virtual network emulation\n", dev);
>          close(fd);
>          return -1;
>      }
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index e14fe36..3d10984 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -171,10 +171,15 @@ static int tap_alloc(char *dev, size_t dev_size)
>      return tap_fd;
>  }
>  
> -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> +int tap_open(char *ifname, int ifname_size, const char *devarg,
> +			int *vnet_hdr, int vnet_hdr_required)
>  {
>      char  dev[10]="";
>      int fd;
> +    if (devarg) {
> +        fprintf(stderr, "dev= not supported\n");
> +        return -1;
> +    }
>      if( (fd = tap_alloc(dev, sizeof(dev))) < 0 ){
>         fprintf(stderr, "Cannot allocate TAP device\n");
>         return -1;
> diff --git a/net/tap.c b/net/tap.c
> index 0d8b424..8635ae2 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -343,12 +343,15 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>  {
>      int fd, vnet_hdr_required;
>      char ifname[128] = {0,};
> +    const char *dev;
>      const char *setup_script;
>  
>      if (qemu_opt_get(opts, "ifname")) {
>          pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
>      }
>  
> +    dev = qemu_opt_get(opts, "dev");
> +
>      *vnet_hdr = qemu_opt_get_bool(opts, "vnet_hdr", 1);
>      if (qemu_opt_get(opts, "vnet_hdr")) {
>          vnet_hdr_required = *vnet_hdr;
> @@ -356,7 +359,8 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>          vnet_hdr_required = 0;
>      }
>  
> -    TFR(fd = tap_open(ifname, sizeof(ifname), vnet_hdr, vnet_hdr_required));
> +    TFR(fd = tap_open(ifname, sizeof(ifname), dev, vnet_hdr,
> +                      vnet_hdr_required));
>      if (fd < 0) {
>          return -1;
>      }
> @@ -382,10 +386,12 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>  
>      if (qemu_opt_get(opts, "fd")) {
>          if (qemu_opt_get(opts, "ifname") ||
> +            qemu_opt_get(opts, "dev") ||
>              qemu_opt_get(opts, "script") ||
>              qemu_opt_get(opts, "downscript") ||
>              qemu_opt_get(opts, "vnet_hdr")) {
> -            qemu_error("ifname=, script=, downscript= and vnet_hdr= is invalid with fd=\n");
> +            qemu_error("ifname=, dev=, script=, downscript= and vnet_hdr= is "
> +                       "invalid with fd=\n");
>              return -1;
>          }
>  
> @@ -425,15 +431,20 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>      if (qemu_opt_get(opts, "fd")) {
>          snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
>      } else {
> -        const char *ifname, *script, *downscript;
> +        const char *ifname, *dev, *script, *downscript;
>  
>          ifname     = qemu_opt_get(opts, "ifname");
> +        dev        = qemu_opt_get(opts, "dev");
>          script     = qemu_opt_get(opts, "script");
>          downscript = qemu_opt_get(opts, "downscript");
>  
>          snprintf(s->nc.info_str, sizeof(s->nc.info_str),
>                   "ifname=%s,script=%s,downscript=%s",
>                   ifname, script, downscript);
> +	if (dev) {
> +		strncat(s->nc.info_str, ",dev=", sizeof(s->nc.info_str));
> +		strncat(s->nc.info_str, dev, sizeof(s->nc.info_str));
> +	}
>  
>          if (strcmp(downscript, "no") != 0) {
>              snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
> diff --git a/net/tap.h b/net/tap.h
> index 538a562..6e76903 100644
> --- a/net/tap.h
> +++ b/net/tap.h
> @@ -34,7 +34,8 @@
>  
>  int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan);
>  
> -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required);
> +int tap_open(char *ifname, int ifname_size, const char *devarg,
> +			int *vnet_hdr, int vnet_hdr_required);
>  
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 1b5781a..586aec3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -810,12 +810,20 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
>      "-net tap[,vlan=n][,name=str],ifname=name\n"
>      "                connect the host TAP network interface to VLAN 'n'\n"
>  #else
> -    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
> +    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name]"
> +#ifdef __linux__
> +                                                       "[,dev=str]"
> +#endif
> +                                                                   "[,script=file]\n"


will be neater if you put [,dev=str] after [,script=file]
Also - it does need a string, but only insofar as all options are strings.
Maybe dev=devfile or dev=file would be clearer.

> +    "        [,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
>      "                connect the host TAP network interface to VLAN 'n' and use the\n"
>      "                network scripts 'file' (default=%s)\n"
>      "                and 'dfile' (default=%s);\n"
>      "                use '[down]script=no' to disable script execution;\n"
>      "                use 'fd=h' to connect to an already opened TAP interface\n"
> +#ifdef __linux__
> +    "                use 'dev=str' to open a specific tap device (default=/dev/net/tun)\n"
> +#endif
>      "                use 'sndbuf=nbytes' to limit the size of the send buffer; the\n"
>      "                default of 'sndbuf=1048576' can be disabled using 'sndbuf=0'\n"
>      "                use vnet_hdr=off to avoid enabling the IFF_VNET_HDR tap flag; use\n"
Arnd Bergmann - Dec. 9, 2009, 3:48 p.m.
On Wednesday 09 December 2009, Michael S. Tsirkin wrote:
> On Wed, Dec 09, 2009 at 03:49:04PM +0100, Arnd Bergmann wrote:
> >  
> > -    TFR(fd = open("/dev/net/tun", O_RDWR));
> > +    if (!*dev)
> > +        dev = "/dev/net/tun";
> > +
> 
> Did you test without dev parameter? I think dev will be NULL
> so this will deference a nullpointer ...
> probably if (!dev) is what you mean?

D'oh. will fix.
  
> will be neater if you put [,dev=str] after [,script=file]
> Also - it does need a string, but only insofar as all options are strings.
> Maybe dev=devfile or dev=file would be clearer.

Yep. Thanks,

	Arnd

Patch

diff --git a/net.c b/net.c
index 13bdbb2..deb12fd 100644
--- a/net.c
+++ b/net.c
@@ -955,6 +955,11 @@  static struct {
             },
 #ifndef _WIN32
             {
+                .name = "dev",
+                .type = QEMU_OPT_STRING,
+                .help = "character device pathname",
+            },
+            {
                 .name = "fd",
                 .type = QEMU_OPT_STRING,
                 .help = "file descriptor of an already opened tap",
diff --git a/net/tap-aix.c b/net/tap-aix.c
index 4bc9f2f..238c190 100644
--- a/net/tap-aix.c
+++ b/net/tap-aix.c
@@ -25,7 +25,8 @@ 
 #include "net/tap.h"
 #include <stdio.h>
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
+int tap_open(char *ifname, int ifname_size, const char *dev,
+			int *vnet_hdr, int vnet_hdr_required)
 {
     fprintf(stderr, "no tap on AIX\n");
     return -1;
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 815997d..8a7334c 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -40,7 +40,8 @@ 
 #include <util.h>
 #endif
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
+int tap_open(char *ifname, int ifname_size, const char *devarg,
+			int *vnet_hdr, int vnet_hdr_required)
 {
     int fd;
     char *dev;
@@ -80,6 +81,12 @@  int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
     }
 #endif
 
+    if (devarg) {
+        qemu_error("dev= not supported\n");
+        close(fd);
+        return -1;
+    }
+
     fstat(fd, &s);
     dev = devname(s.st_rdev, S_IFCHR);
     pstrcpy(ifname, ifname_size, dev);
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 6af9e82..d6831c0 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -32,14 +32,18 @@ 
 #include "sysemu.h"
 #include "qemu-common.h"
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
+int tap_open(char *ifname, int ifname_size, const char *dev,
+                        int *vnet_hdr, int vnet_hdr_required)
 {
     struct ifreq ifr;
     int fd, ret;
 
-    TFR(fd = open("/dev/net/tun", O_RDWR));
+    if (!*dev)
+        dev = "/dev/net/tun";
+
+    TFR(fd = open(dev, O_RDWR));
     if (fd < 0) {
-        fprintf(stderr, "warning: could not open /dev/net/tun: no virtual network emulation\n");
+        fprintf(stderr, "warning: could not open %s: no virtual network emulation\n", dev);
         return -1;
     }
     memset(&ifr, 0, sizeof(ifr));
@@ -70,7 +74,7 @@  int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
         pstrcpy(ifr.ifr_name, IFNAMSIZ, "tap%d");
     ret = ioctl(fd, TUNSETIFF, (void *) &ifr);
     if (ret != 0) {
-        fprintf(stderr, "warning: could not configure /dev/net/tun: no virtual network emulation\n");
+        fprintf(stderr, "warning: could not configure %s: no virtual network emulation\n", dev);
         close(fd);
         return -1;
     }
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index e14fe36..3d10984 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -171,10 +171,15 @@  static int tap_alloc(char *dev, size_t dev_size)
     return tap_fd;
 }
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
+int tap_open(char *ifname, int ifname_size, const char *devarg,
+			int *vnet_hdr, int vnet_hdr_required)
 {
     char  dev[10]="";
     int fd;
+    if (devarg) {
+        fprintf(stderr, "dev= not supported\n");
+        return -1;
+    }
     if( (fd = tap_alloc(dev, sizeof(dev))) < 0 ){
        fprintf(stderr, "Cannot allocate TAP device\n");
        return -1;
diff --git a/net/tap.c b/net/tap.c
index 0d8b424..8635ae2 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -343,12 +343,15 @@  static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
 {
     int fd, vnet_hdr_required;
     char ifname[128] = {0,};
+    const char *dev;
     const char *setup_script;
 
     if (qemu_opt_get(opts, "ifname")) {
         pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
     }
 
+    dev = qemu_opt_get(opts, "dev");
+
     *vnet_hdr = qemu_opt_get_bool(opts, "vnet_hdr", 1);
     if (qemu_opt_get(opts, "vnet_hdr")) {
         vnet_hdr_required = *vnet_hdr;
@@ -356,7 +359,8 @@  static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
         vnet_hdr_required = 0;
     }
 
-    TFR(fd = tap_open(ifname, sizeof(ifname), vnet_hdr, vnet_hdr_required));
+    TFR(fd = tap_open(ifname, sizeof(ifname), dev, vnet_hdr,
+                      vnet_hdr_required));
     if (fd < 0) {
         return -1;
     }
@@ -382,10 +386,12 @@  int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
 
     if (qemu_opt_get(opts, "fd")) {
         if (qemu_opt_get(opts, "ifname") ||
+            qemu_opt_get(opts, "dev") ||
             qemu_opt_get(opts, "script") ||
             qemu_opt_get(opts, "downscript") ||
             qemu_opt_get(opts, "vnet_hdr")) {
-            qemu_error("ifname=, script=, downscript= and vnet_hdr= is invalid with fd=\n");
+            qemu_error("ifname=, dev=, script=, downscript= and vnet_hdr= is "
+                       "invalid with fd=\n");
             return -1;
         }
 
@@ -425,15 +431,20 @@  int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
     if (qemu_opt_get(opts, "fd")) {
         snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
     } else {
-        const char *ifname, *script, *downscript;
+        const char *ifname, *dev, *script, *downscript;
 
         ifname     = qemu_opt_get(opts, "ifname");
+        dev        = qemu_opt_get(opts, "dev");
         script     = qemu_opt_get(opts, "script");
         downscript = qemu_opt_get(opts, "downscript");
 
         snprintf(s->nc.info_str, sizeof(s->nc.info_str),
                  "ifname=%s,script=%s,downscript=%s",
                  ifname, script, downscript);
+	if (dev) {
+		strncat(s->nc.info_str, ",dev=", sizeof(s->nc.info_str));
+		strncat(s->nc.info_str, dev, sizeof(s->nc.info_str));
+	}
 
         if (strcmp(downscript, "no") != 0) {
             snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
diff --git a/net/tap.h b/net/tap.h
index 538a562..6e76903 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -34,7 +34,8 @@ 
 
 int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan);
 
-int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required);
+int tap_open(char *ifname, int ifname_size, const char *devarg,
+			int *vnet_hdr, int vnet_hdr_required);
 
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 1b5781a..586aec3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -810,12 +810,20 @@  DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net tap[,vlan=n][,name=str],ifname=name\n"
     "                connect the host TAP network interface to VLAN 'n'\n"
 #else
-    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
+    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name]"
+#ifdef __linux__
+                                                       "[,dev=str]"
+#endif
+                                                                   "[,script=file]\n"
+    "        [,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
     "                connect the host TAP network interface to VLAN 'n' and use the\n"
     "                network scripts 'file' (default=%s)\n"
     "                and 'dfile' (default=%s);\n"
     "                use '[down]script=no' to disable script execution;\n"
     "                use 'fd=h' to connect to an already opened TAP interface\n"
+#ifdef __linux__
+    "                use 'dev=str' to open a specific tap device (default=/dev/net/tun)\n"
+#endif
     "                use 'sndbuf=nbytes' to limit the size of the send buffer; the\n"
     "                default of 'sndbuf=1048576' can be disabled using 'sndbuf=0'\n"
     "                use vnet_hdr=off to avoid enabling the IFF_VNET_HDR tap flag; use\n"