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

[Michael S. Tsirkin]

On Tue, Dec 08, 2009 at 06:41:44PM +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.
> 
> Signed-off-by: Arnd Bergmann <address@hidden>
> ---
> 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..b789d06 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, char *dev, int dev_size,
> +                     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..09a7780 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, char *dev, int dev_size,
> +                     int *vnet_hdr, int vnet_hdr_required)
>  {
>      int fd;
>      char *dev;

Does this compile?

> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 6af9e82..a6df123 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -32,14 +32,21 @@
>  #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, char *dev, int dev_size,

dev is never modified, so it should be const char *.

> +                     int *vnet_hdr, int vnet_hdr_required)
>  {
>      struct ifreq ifr;
>      int fd, ret;
> +    const char *path;
>  
> -    TFR(fd = open("/dev/net/tun", O_RDWR));
> +    if (dev[0] == '\0')

== 0 checks are ugly. if (*dev) is shorter, and it's a standard C
idiom to detect non-empty string. Or better pass NULL if no device,
then you can just path = dev ? dev : "/dev/net/tun".

> +     path = "/dev/net/tun";
> +    else
> +     path = dev;

Please do not indent by single space.

> +
> +    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", path);
>          return -1;
>      }
>      memset(&ifr, 0, sizeof(ifr));
> @@ -70,7 +77,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", path);
>          close(fd);
>          return -1;
>      }
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index e14fe36..72abb78 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -171,7 +171,8 @@ 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, char *dev, int dev_size,
> +                     int *vnet_hdr, int vnet_hdr_required)
>  {
>      char  dev[10]="";
>      int fd;

Does this compile?

> diff --git a/net/tap.c b/net/tap.c
> index 0d8b424..14ddf65 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -343,12 +343,17 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>  {
>      int fd, vnet_hdr_required;
>      char ifname[128] = {0,};
> +    char dev[128] = {0,};
>      const char *setup_script;
>  
>      if (qemu_opt_get(opts, "ifname")) {
>          pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
>      }
>  
> +    if (qemu_opt_get(opts, "dev")) {
> +        pstrcpy(dev, sizeof(dev), qemu_opt_get(opts, "dev"));
> +    }
> +

While in this case this is unlikely to be a problem in practice, we still
should not add arbitrary limitations on file name lengths.  Just teach
tap_open to get NULL instead of and empty string, or better supply
default /dev/net/tun to the option, and you will not need the strcpy.


>      *vnet_hdr = qemu_opt_get_bool(opts, "vnet_hdr", 1);
>      if (qemu_opt_get(opts, "vnet_hdr")) {
>          vnet_hdr_required = *vnet_hdr;
> @@ -356,7 +361,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, sizeof(dev),
> +                     vnet_hdr, vnet_hdr_required));
>      if (fd < 0) {
>          return -1;
>      }
> @@ -371,6 +377,7 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>      }
>  
>      qemu_opt_set(opts, "ifname", ifname);
> +    qemu_opt_set(opts, "dev", dev);
>  
>      return fd;
>  }
> @@ -382,10 +389,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 +434,16 @@ 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);
> +                 "ifname=%s,dev=%s,script=%s,downscript=%s",

This will look strange if dev is not supplied, will it not?
Also, I wonder whether there might be any scripts parsing
info string from monitor, that will get broken by the
extra parameter. How about we only print dev if it
is not the default?

> +                 ifname, dev, script, downscript);
>  
>          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..ba44363 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, char *dev, int dev_size,
> +                     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..7f7aa18 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -810,12 +810,14 @@ 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][,dev=str][,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"
> +    "                use 'dev=str' to open a specific tap character device\n"

please document default /dev/net/tun

>      "                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"
>