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

[Jason Wang <jasowang@redhat.com>]

On Tue, Dec 23, 2014 at 4:44 PM, Roy Vardi <royv@ezchip.com> wrote:
> 
> 
>>  -----Original Message-----
>>  From: Jason Wang [mailto:jasowang@redhat.com]
>>  Sent: Monday, December 22, 2014 8:33 AM
>>  To: Roy Vardi; qemu-devel@nongnu.org
>>  Cc: aliguori@amazon.com; armbru@redhat.com; lcapitulino@redhat.com;
>>  Noam Camus; stefanha@redhat.com
>>  Subject: Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net 
>> tap option
>>  
>>  
>>  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?
> 
> We're running qemu as users, non-privilege... 
> Our work flow includes:
> 1. Running an internal tool for opening a persistent tap interface 
> 2. Running qemu with the tap interface we got from above
> Our environment includes a lot of such qemu runs, and we want to 
> avoid "zombie" tap interfaces, and we achieve it with this new flag 
> I've added.

I get the case, thanks for the explaining. But qemu has already had
method to solve this. Try downscript for tap, this external script can
do cleanup before closing tap fd.

E.g. in your case, you may need to run tunctl -d.
> 
> It's also worth mentioning that we're able to configure the tap 
> interface in qemu through the ioctl as users (non-privileged).
> If the persistent flag is not given (or if it's given as on), I'm not 
> actively doing anything, the same code that you're familiar with will 
> run. My change only affects the case where "persistent=off" is given

Right, but it was not elegant to do this during tap_open. A more
sutiable place is tap_cleanup(). Since qemu has already had
downscript which can do even more complex things, there's no need for
qemu to handle this specific case by itself.
> 
> 
>>  >     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?
> 
> I'm using qemu only on linux, so I can't really tell.
> I would appreciate it if you could perhaps tell me how can I make 
> sure I didn't break anything 

E.g only linux version of tap_open() can accept persist_required. This
will break the build of other platforms for sure.
> 
> 
>>  >
>>  > 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.
> 
> Sure, I'll fix and will send another patch
> 
>>  >
>>  >  #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][,down
>>  
>> script=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][,down
>>  
>> script=dfile][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off
>>  
>> ][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n][,persistent=o
>>  n|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.
> 
> I didn't change any legacy behavior.
> If the persistent flag is not given, then the code that will be 
> executed is as it is today.
> I'm only actively changing (configuring) the interface if 
> "persistent=off" is given

Yes, I see.

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