[Qemu-devel] Re: [PATCH, try 2, version 2] qemu/tap: add -net tap, dev= option

[Michael S. Tsirkin]

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 <address@hidden>
> Acked-by: Mark McLoughlin <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> ---
> 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"