Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option

[Jason Wang <jasowang@redhat.com>]

On 12/21/2014 03:48 PM, Roy Vardi wrote:
> From: Roy Vardi <royv@ezchip.com>
>
>     Add 'persistent' boolean flag to -net tap option.
>     When set to off - tap interface will be released on shutdown
>     When set to on\not specified - tap interface will remain

I'm interested of the user cases in the case. Usually, persistent flag
was used to let privileged application to create/configure the device
and then it could be used by non-privileged users. If qemu has already
had privilege, why need set persistent in this case?
>     Running with -net tap,persistent=off will force the tap interface
>     down when qemu goes down, thus ensuring that there're no zombie tap
>     interfaces left
>
>     This is achieved using another ioctl
>
>     Note: This commit includes the above support only for linux systems

But your patch will break non-linux builds, no?
>
> Signed-off-by: Roy Vardi <royv@ezchip.com>
> ---
>  net/tap-linux.c  |   14 +++++++++++++-
>  net/tap.c        |   10 ++++++----
>  net/tap_int.h    |    2 +-
>  qapi-schema.json |    5 ++++-
>  qemu-options.hx  |    3 ++-
>  5 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 812bf2d..7a98390 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -29,6 +29,7 @@
>  
>  #include <net/if.h>
>  #include <sys/ioctl.h>
> +#include <linux/if_tun.h>

To reduce headers dependency, we put tun flags in net/tap-linux.h.
>  
>  #include "sysemu/sysemu.h"
>  #include "qemu-common.h"
> @@ -37,7 +38,7 @@
>  #define PATH_NET_TUN "/dev/net/tun"
>  
>  int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
> -             int vnet_hdr_required, int mq_required)
> +             int vnet_hdr_required, int mq_required, int persistent_required)
>  {
>      struct ifreq ifr;
>      int fd, ret;
> @@ -109,6 +110,17 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>          close(fd);
>          return -1;
>      }
> +
> +    if (!persistent_required) {
> +        ret = ioctl(fd, TUNSETPERSIST, 0);
> +        if (ret != 0) {
> +            error_report("could not configure non-persistent %s (%s): %m",
> +                        PATH_NET_TUN, ifr.ifr_name);
> +            close(fd);
> +            return -1;
> +        }
> +    }
> +
>      pstrcpy(ifname, ifname_size, ifr.ifr_name);
>      fcntl(fd, F_SETFL, O_NONBLOCK);
>      return fd;
> diff --git a/net/tap.c b/net/tap.c
> index bde6b58..055863a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -556,7 +556,8 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>  
>  static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>                          const char *setup_script, char *ifname,
> -                        size_t ifname_sz, int mq_required)
> +                        size_t ifname_sz, int mq_required,
> +                        int persistent_required)
>  {
>      int fd, vnet_hdr_required;
>  
> @@ -569,7 +570,7 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>      }
>  
>      TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
> -                      mq_required));
> +                      mq_required, persistent_required));
>      if (fd < 0) {
>          return -1;
>      }
> @@ -688,7 +689,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                   NetClientState *peer)
>  {
>      const NetdevTapOptions *tap;
> -    int fd, vnet_hdr = 0, i = 0, queues;
> +    int fd, vnet_hdr = 0, i = 0, queues, persistent;
>      /* for the no-fd, no-helper case */
>      const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
>      const char *downscript = NULL;
> @@ -699,6 +700,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>      tap = opts->tap;
>      queues = tap->has_queues ? tap->queues : 1;
>      vhostfdname = tap->has_vhostfd ? tap->vhostfd : NULL;
> +    persistent = tap->has_persistent ? tap->persistent : 1;
>  
>      /* QEMU vlans does not support multiqueue tap, in this case peer is set.
>       * For -netdev, peer is always NULL. */
> @@ -816,7 +818,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>  
>          for (i = 0; i < queues; i++) {
>              fd = net_tap_init(tap, &vnet_hdr, i >= 1 ? "no" : script,
> -                              ifname, sizeof ifname, queues > 1);
> +                              ifname, sizeof ifname, queues > 1, persistent);
>              if (fd == -1) {
>                  return -1;
>              }
> diff --git a/net/tap_int.h b/net/tap_int.h
> index 79afdf2..0eb2168 100644
> --- a/net/tap_int.h
> +++ b/net/tap_int.h
> @@ -30,7 +30,7 @@
>  #include "qapi-types.h"
>  
>  int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
> -             int vnet_hdr_required, int mq_required);
> +             int vnet_hdr_required, int mq_required, int persistent_required);
>  
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 563b4ad..99e6482 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2007,6 +2007,8 @@
>  #
>  # @queues: #optional number of queues to be created for multiqueue capable tap
>  #
> +# @persistent: #optional for opening tap in persistent mode (default: on) (Since: 2.3)
> +#
>  # Since 1.2
>  ##
>  { 'type': 'NetdevTapOptions',
> @@ -2023,7 +2025,8 @@
>      '*vhostfd':    'str',
>      '*vhostfds':   'str',
>      '*vhostforce': 'bool',
> -    '*queues':     'uint32'} }
> +    '*queues':     'uint32',
> +    '*persistent': 'bool'} }
>  
>  ##
>  # @NetdevSocketOptions
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 10b9568..d8c033d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1417,7 +1417,7 @@ 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][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n"
> +    "-net tap[,vlan=n][,name=str][,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n][,persistent=on|off]\n"
>      "                connect the host TAP network interface to VLAN 'n'\n"
>      "                use network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT ")\n"
>      "                to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
> @@ -1437,6 +1437,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
>      "                use 'vhostfd=h' to connect to an already opened vhost net device\n"
>      "                use 'vhostfds=x:y:...:z to connect to multiple already opened vhost net devices\n"
>      "                use 'queues=n' to specify the number of queues to be created for multiqueue TAP\n"
> +    "                use 'persistent=off' to release the TAP interface on shutdown (default=on)\n"

As Stefan mentioned, we don't set persistent by default in the past. So
please don't change this behaviour.
>      "-net bridge[,vlan=n][,name=str][,br=bridge][,helper=helper]\n"
>      "                connects a host TAP network interface to a host bridge device 'br'\n"
>      "                (default=" DEFAULT_BRIDGE_INTERFACE ") using the program 'helper'\n"