* [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
@ 2014-12-15 12:05 Roy Vardi
2014-12-19 13:13 ` Stefan Hajnoczi
2014-12-19 15:29 ` Eric Blake
0 siblings, 2 replies; 14+ messages in thread
From: Roy Vardi @ 2014-12-15 12:05 UTC (permalink / raw
To: qemu-devel; +Cc: aliguori, royv, armbru, lcapitulino, stefanha
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
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
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..2072166 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>
#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..43267bb 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_reuired)
{
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_reuired));
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..e4ea73f 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
+#
# 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..d26215a 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"
"-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"
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-15 12:05 Roy Vardi
@ 2014-12-19 13:13 ` Stefan Hajnoczi
2014-12-19 13:18 ` Daniel P. Berrange
2014-12-21 7:17 ` Roy Vardi
2014-12-19 15:29 ` Eric Blake
1 sibling, 2 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2014-12-19 13:13 UTC (permalink / raw
To: Roy Vardi; +Cc: lcapitulino, stefanha, qemu-devel, aliguori, armbru
[-- Attachment #1: Type: text/plain, Size: 3892 bytes --]
On Mon, Dec 15, 2014 at 02:05:23PM +0200, 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
>
> 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
I don't understand the point of this patch. The following doesn't
persist the tun interface:
qemu-system-i386 -net tap,script=myscript.sh,downscript=no -net nic
You are changing the default to persist the interface, won't this cause
problems for existing users who don't expect persistent interfaces?
> @@ -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) {
Indentation is off here. QEMU uses 4-space indentation and this if
statement should not be indented.
> + 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..43267bb 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_reuired)
Typo s/reuired/required/
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 10b9568..d26215a 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"
Missing comma:
[,persistent=on|off]
> " 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"
Please use the same formatting as for the other options:
use 'persistent=off' to release ...
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-19 13:13 ` Stefan Hajnoczi
@ 2014-12-19 13:18 ` Daniel P. Berrange
2014-12-21 7:17 ` Roy Vardi
1 sibling, 0 replies; 14+ messages in thread
From: Daniel P. Berrange @ 2014-12-19 13:18 UTC (permalink / raw
To: Stefan Hajnoczi
Cc: stefanha, Roy Vardi, armbru, qemu-devel, aliguori, lcapitulino
On Fri, Dec 19, 2014 at 01:13:50PM +0000, Stefan Hajnoczi wrote:
> On Mon, Dec 15, 2014 at 02:05:23PM +0200, 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
> >
> > 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
>
> I don't understand the point of this patch. The following doesn't
> persist the tun interface:
>
> qemu-system-i386 -net tap,script=myscript.sh,downscript=no -net nic
>
> You are changing the default to persist the interface, won't this cause
> problems for existing users who don't expect persistent interfaces?
Yea, changing the default to persist interfaces is going to cause existing
apps using this syntax to leak these tap devices on QEMU shutdown.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-15 12:05 Roy Vardi
2014-12-19 13:13 ` Stefan Hajnoczi
@ 2014-12-19 15:29 ` Eric Blake
1 sibling, 0 replies; 14+ messages in thread
From: Eric Blake @ 2014-12-19 15:29 UTC (permalink / raw
To: Roy Vardi, qemu-devel; +Cc: armbru, stefanha, aliguori, lcapitulino
On 12/15/2014 05:05 AM, 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
>
> 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
>
> Signed-off-by: Roy Vardi <royv@ezchip.com>
> ---
> +++ 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
Missing a '(since 2.3)' designator; also might be worth mentioning that
the default is true if omitted.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-19 13:13 ` Stefan Hajnoczi
2014-12-19 13:18 ` Daniel P. Berrange
@ 2014-12-21 7:17 ` Roy Vardi
2015-01-06 11:58 ` Stefan Hajnoczi
1 sibling, 1 reply; 14+ messages in thread
From: Roy Vardi @ 2014-12-21 7:17 UTC (permalink / raw
To: Stefan Hajnoczi
Cc: lcapitulino@redhat.com, stefanha@redhat.com,
qemu-devel@nongnu.org, aliguori@amazon.com, armbru@redhat.com
[-- Attachment #1: Type: text/plain, Size: 5518 bytes --]
Hi Stefan,
When using "-net tap" flag today, the tap interface is not released on shutdown. This is the default behavior and I'm not breaking it.
Running with "-net tap persistent=off " will cause the interface to be released on shutdown, and running with "-net tap persistent=on " (or without the persistent flag at all), the behavior remains the same (i.e - interface won't be released. This is the status in qemu today, I didn't change any legacy command-lines)
Example for a command line:
. /qemu-system -net tap,ifname=tap0,script=no,downscript=no,persistent=off
. /qemu-system -net tap,ifname=tap0,script=no,downscript=no,persistent=on
. /qemu-system -net tap,ifname=tap0,script=no,downscript=no
The last two lines are identical as far as releasing the tap is concerned.
This is relevant for users who want their tap interfaces to be released on qemu shutdown. The rest of the users won't be influenced by this change.
I'll fix the spelling and format bugs and will send a new patch soon.
Thanks for your review and comments,
Roy Vardi
-----Original Message-----
From: Stefan Hajnoczi [mailto:stefanha@gmail.com]
Sent: Friday, December 19, 2014 3:14 PM
To: Roy Vardi
Cc: qemu-devel@nongnu.org; aliguori@amazon.com; armbru@redhat.com; lcapitulino@redhat.com; stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
On Mon, Dec 15, 2014 at 02:05:23PM +0200, Roy Vardi wrote:
> From: Roy Vardi <royv@ezchip.com<mailto: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
>
> 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
I don't understand the point of this patch. The following doesn't persist the tun interface:
qemu-system-i386 -net tap,script=myscript.sh,downscript=no -net nic
You are changing the default to persist the interface, won't this cause problems for existing users who don't expect persistent interfaces?
> @@ -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) {
Indentation is off here. QEMU uses 4-space indentation and this if statement should not be indented.
> + 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..43267bb 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_reuired)
Typo s/reuired/required/
> diff --git a/qemu-options.hx b/qemu-options.hx index 10b9568..d26215a
> 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"
Missing comma:
[,persistent=on|off]
> " 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"
Please use the same formatting as for the other options:
use 'persistent=off' to release ...
[-- Attachment #2: Type: text/html, Size: 24843 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
@ 2014-12-21 7:48 Roy Vardi
2014-12-22 6:33 ` Jason Wang
2015-01-22 15:25 ` Eric Blake
0 siblings, 2 replies; 14+ messages in thread
From: Roy Vardi @ 2014-12-21 7:48 UTC (permalink / raw
To: qemu-devel; +Cc: aliguori, royv, armbru, lcapitulino, noamc, stefanha
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
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
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>
#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"
"-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"
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-21 7:48 [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option Roy Vardi
@ 2014-12-22 6:33 ` Jason Wang
2014-12-23 8:44 ` Roy Vardi
2015-01-22 15:25 ` Eric Blake
1 sibling, 1 reply; 14+ messages in thread
From: Jason Wang @ 2014-12-22 6:33 UTC (permalink / raw
To: Roy Vardi, qemu-devel; +Cc: stefanha, noamc, armbru, aliguori, lcapitulino
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"
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-22 6:33 ` Jason Wang
@ 2014-12-23 8:44 ` Roy Vardi
[not found] ` <AM2PR02MB0532BC1703B005CE79A141FCAA570@AM2PR02MB0532.eurprd02.prod.outlook .com>
0 siblings, 1 reply; 14+ messages in thread
From: Roy Vardi @ 2014-12-23 8:44 UTC (permalink / raw
To: Jason Wang, qemu-devel@nongnu.org
Cc: stefanha@redhat.com, Noam Camus, armbru@redhat.com,
aliguori@amazon.com, lcapitulino@redhat.com
> -----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.
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
> > 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
> >
> > 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
> > "-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"
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
[not found] ` <AM2PR02MB0532BC1703B005CE79A141FCAA570@AM2PR02MB0532.eurprd02.prod.outlook .com>
@ 2014-12-23 9:13 ` Jason Wang
2014-12-29 7:38 ` Roy Vardi
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2014-12-23 9:13 UTC (permalink / raw
To: Roy Vardi
Cc: stefanha@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
lcapitulino@redhat.com, Noam Camus, aliguori@amazon.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"
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-23 9:13 ` Jason Wang
@ 2014-12-29 7:38 ` Roy Vardi
2015-01-04 7:28 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Roy Vardi @ 2014-12-29 7:38 UTC (permalink / raw
To: Jason Wang
Cc: stefanha@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
lcapitulino@redhat.com, Noam Camus, aliguori@amazon.com
[-- Attachment #1: Type: text/plain, Size: 13540 bytes --]
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Tuesday, December 23, 2014 11:13 AM
> To: Roy Vardi
> Cc: qemu-devel@nongnu.org; stefanha@redhat.com; Noam Camus;
> armbru@redhat.com; aliguori@amazon.com; lcapitulino@redhat.com
> Subject: Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
>
>
>
> On Tue, Dec 23, 2014 at 4:44 PM, Roy Vardi <royv@ezchip.com<mailto: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<mailto:qemu-devel@nongnu.org>
> >> Cc: aliguori@amazon.com<mailto:aliguori@amazon.com>; armbru@redhat.com<mailto:armbru@redhat.com>; lcapitulino@redhat.com<mailto:lcapitulino@redhat.com>;
> >> Noam Camus; stefanha@redhat.com<mailto: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<mailto: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.
Thanks for the reference.
I've checked the downscript option, but found it unsuitable for us: Qemu runs the downscript before closing the fd, so a script which removes the interface will fail due to busy device.
I've changed the order in the qemu code so that the script is called after the descriptor is closed and it works for us.
What do you think about the following patch:
--- a/net/tap.c
+++ b/net/tap.c
@@ -296,12 +296,11 @@ static void tap_cleanup(NetClientState *nc)
qemu_purge_queued_packets(nc);
- if (s->down_script[0])
- launch_script(s->down_script, s->down_script_arg, s->fd);
-
tap_read_poll(s, false);
tap_write_poll(s, false);
close(s->fd);
+ if (s->down_script[0])
+ launch_script(s->down_script, s->down_script_arg, s->fd);
s->fd = -1;
}
?
Thanks,
Roy Vardi
> >
> > 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<mailto: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][,vhos
> >> t=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][,vhos
> >> t=on|off
> >>
> >> ][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n][,pe
> >> rsistent=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"
> >
> >
[-- Attachment #2: Type: text/html, Size: 66412 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-29 7:38 ` Roy Vardi
@ 2015-01-04 7:28 ` Jason Wang
2015-01-18 9:42 ` Roy Vardi
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2015-01-04 7:28 UTC (permalink / raw
To: Roy Vardi
Cc: stefanha@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
lcapitulino@redhat.com, Noam Camus, aliguori@amazon.com
On 12/29/2014 03:38 PM, Roy Vardi wrote:
>
>
>
>
>
> > -----Original Message-----
>
> > From: Jason Wang [mailto:jasowang@redhat.com]
>
> > Sent: Tuesday, December 23, 2014 11:13 AM
>
> > To: Roy Vardi
>
> > Cc: qemu-devel@nongnu.org; stefanha@redhat.com; Noam Camus;
>
> > armbru@redhat.com; aliguori@amazon.com; lcapitulino@redhat.com
>
> > Subject: Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net
> tap option
>
> >
>
> >
>
> >
>
> > On Tue, Dec 23, 2014 at 4:44 PM, Roy Vardi <royv@ezchip.com
> <mailto: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 <mailto:qemu-devel@nongnu.org>
>
> > >> Cc: aliguori@amazon.com <mailto:aliguori@amazon.com>;
> armbru@redhat.com <mailto:armbru@redhat.com>; lcapitulino@redhat.com
> <mailto:lcapitulino@redhat.com>;
>
> > >> Noam Camus; stefanha@redhat.com <mailto: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 <mailto: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.
>
>
>
> Thanks for the reference.
>
> I've checked the downscript option, but found it unsuitable for us:
> Qemu runs the downscript before closing the fd, so a script which
> removes the interface will fail due to busy device.
>
Right, tunctl needs another TUNSETIFF ioctl(). You may want to delete it
through ip link del link dev $1 in your qemu-ifdown.
>
> I've changed the order in the qemu code so that the script is called
> after the descriptor is closed and it works for us.
>
>
>
> What do you think about the following patch:
>
>
>
This change behaviour which may break existing down scripts.
Thanks
>
> --- a/net/tap.c
>
> +++ b/net/tap.c
>
> @@ -296,12 +296,11 @@ static void tap_cleanup(NetClientState *nc)
>
> qemu_purge_queued_packets(nc);
>
> - if (s->down_script[0])
>
> - launch_script(s->down_script, s->down_script_arg, s->fd);
>
> -
>
> tap_read_poll(s, false);
>
> tap_write_poll(s, false);
>
> close(s->fd);
>
> + if (s->down_script[0])
>
> + launch_script(s->down_script, s->down_script_arg, s->fd);
>
> s->fd = -1;
>
> }
>
>
>
> ?
>
>
>
> Thanks,
>
> Roy Vardi
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-21 7:17 ` Roy Vardi
@ 2015-01-06 11:58 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2015-01-06 11:58 UTC (permalink / raw
To: Roy Vardi
Cc: lcapitulino@redhat.com, stefanha@redhat.com,
qemu-devel@nongnu.org, aliguori@amazon.com, armbru@redhat.com
[-- Attachment #1: Type: text/plain, Size: 678 bytes --]
On Sun, Dec 21, 2014 at 07:17:42AM +0000, Roy Vardi wrote:
> When using "-net tap" flag today, the tap interface is not released on shutdown. This is the default behavior and I'm not breaking it.
...
> . /qemu-system -net tap,ifname=tap0,script=no,downscript=no
That is not the behavior that I observe with Linux
3.17.4-302.fc21.x86_64 and qemu.git/master
(ab0302ee764fd702465aef6d88612cdff4302809).
When I run your command-line, the tap0 interface is visible.
After QEMU shuts down the tap interface is no longer visible in the
output of the "ip addr" command.
What do you observe on your machine that makes you say "the tap
interface is not released on shutdown"?
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2015-01-04 7:28 ` Jason Wang
@ 2015-01-18 9:42 ` Roy Vardi
0 siblings, 0 replies; 14+ messages in thread
From: Roy Vardi @ 2015-01-18 9:42 UTC (permalink / raw
To: Jason Wang
Cc: stefanha@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
lcapitulino@redhat.com, Noam Camus, aliguori@amazon.com
> On 12/29/2014 03:38 PM, Roy Vardi wrote:
> >
> >
> >
> >
> >
> > > -----Original Message-----
> >
> > > From: Jason Wang [mailto:jasowang@redhat.com]
> >
> > > Sent: Tuesday, December 23, 2014 11:13 AM
> >
> > > To: Roy Vardi
> >
> > > Cc: qemu-devel@nongnu.org; stefanha@redhat.com; Noam Camus;
> >
> > > armbru@redhat.com; aliguori@amazon.com; lcapitulino@redhat.com
> >
> > > Subject: Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net
> > tap option
> >
> > >
> >
> > >
> >
> > >
> >
> > > On Tue, Dec 23, 2014 at 4:44 PM, Roy Vardi <royv@ezchip.com
> > <mailto: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
> > > >> <mailto:qemu-devel@nongnu.org>
> >
> > > >> Cc: aliguori@amazon.com <mailto:aliguori@amazon.com>;
> > armbru@redhat.com <mailto:armbru@redhat.com>;
> lcapitulino@redhat.com
> > <mailto:lcapitulino@redhat.com>;
> >
> > > >> Noam Camus; stefanha@redhat.com <mailto: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 <mailto: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.
> >
> >
> >
> > Thanks for the reference.
> >
> > I've checked the downscript option, but found it unsuitable for us:
> > Qemu runs the downscript before closing the fd, so a script which
> > removes the interface will fail due to busy device.
> >
>
> Right, tunctl needs another TUNSETIFF ioctl(). You may want to delete it through
> ip link del link dev $1 in your qemu-ifdown.
Yes, this works, thanks!
> >
> > I've changed the order in the qemu code so that the script is called
> > after the descriptor is closed and it works for us.
> >
> >
> >
> > What do you think about the following patch:
> >
> >
> >
>
> This change behaviour which may break existing down scripts.
>
> Thanks
> >
> > --- a/net/tap.c
> >
> > +++ b/net/tap.c
> >
> > @@ -296,12 +296,11 @@ static void tap_cleanup(NetClientState *nc)
> >
> > qemu_purge_queued_packets(nc);
> >
> > - if (s->down_script[0])
> >
> > - launch_script(s->down_script, s->down_script_arg, s->fd);
> >
> > -
> >
> > tap_read_poll(s, false);
> >
> > tap_write_poll(s, false);
> >
> > close(s->fd);
> >
> > + if (s->down_script[0])
> >
> > + launch_script(s->down_script, s->down_script_arg, s->fd);
> >
> > s->fd = -1;
> >
> > }
> >
> >
> >
> > ?
> >
> >
> >
> > Thanks,
> >
> > Roy Vardi
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option
2014-12-21 7:48 [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option Roy Vardi
2014-12-22 6:33 ` Jason Wang
@ 2015-01-22 15:25 ` Eric Blake
1 sibling, 0 replies; 14+ messages in thread
From: Eric Blake @ 2015-01-22 15:25 UTC (permalink / raw
To: Roy Vardi, qemu-devel; +Cc: armbru, noamc, stefanha, aliguori, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 3161 bytes --]
On 12/21/2014 12:48 AM, 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
>
> Running with -net tap,persistent=off will force the tap interface
> down when qemu goes down, thus ensuring that there're no zombie tap
s/there're/there are/
> interfaces left
>
> This is achieved using another ioctl
>
> Note: This commit includes the above support only for linux systems
>
> Signed-off-by: Roy Vardi <royv@ezchip.com>
> ---
> #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)
Used as a boolean, so s/int/bool/
> {
> 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",
%m is not portable to non-glibc (then again, this file is Linux-only).
> +++ 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)
Again, prefer bool.
> @@ -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;
Use bool.
> /* 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;
s/1/true/
> +++ 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)
Long line; please wrap at 80 columns. In QMP, the default is 'true',
not 'on' (the command line parser maps multiple strings including 'on'
into the single QMP bool type).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-01-22 15:25 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-21 7:48 [Qemu-devel] [PATCH] net: Add persistent flag to -net tap option Roy Vardi
2014-12-22 6:33 ` Jason Wang
2014-12-23 8:44 ` Roy Vardi
[not found] ` <AM2PR02MB0532BC1703B005CE79A141FCAA570@AM2PR02MB0532.eurprd02.prod.outlook .com>
2014-12-23 9:13 ` Jason Wang
2014-12-29 7:38 ` Roy Vardi
2015-01-04 7:28 ` Jason Wang
2015-01-18 9:42 ` Roy Vardi
2015-01-22 15:25 ` Eric Blake
-- strict thread matches above, loose matches on Subject: below --
2014-12-15 12:05 Roy Vardi
2014-12-19 13:13 ` Stefan Hajnoczi
2014-12-19 13:18 ` Daniel P. Berrange
2014-12-21 7:17 ` Roy Vardi
2015-01-06 11:58 ` Stefan Hajnoczi
2014-12-19 15:29 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.