[Qemu-devel] [PATCH 0/3] Some qemu-bridge-helper work

[Qemu-devel] [PATCH 0/3] Some qemu-bridge-helper work

[Markus Armbruster]

Markus Armbruster (3):
  MAINTAINERS: Add qemu-bridge-helper.c to "Network device backends"
  net: Deprecate tap backend's parameter "helper"
  qemu-bridge-helper: Document known shortcomings

 MAINTAINERS          |  1 +
 qapi/net.json        |  3 ++-
 qemu-bridge-helper.c | 12 +++++++++++-
 qemu-deprecated.texi |  4 ++++
 qemu-options.hx      | 18 ++----------------
 5 files changed, 20 insertions(+), 18 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/3] MAINTAINERS: Add qemu-bridge-helper.c to "Network device backends"

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a96829ea83..8b73f1f0d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1936,6 +1936,7 @@ M: Jason Wang <jasowang@redhat.com>
 S: Maintained
 F: net/
 F: include/net/
+F: qemu-bridge-helper.c
 T: git https://github.com/jasowang/qemu.git net
 F: qapi/net.json
 
-- 
2.21.0

[Qemu-devel] [PATCH 2/3] net: Deprecate tap backend's parameter "helper"

[Markus Armbruster]

-netdev tap,helper=... is a useless duplicate of -netdev bridge.
Deprecate and de-document.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/net.json        |  3 ++-
 qemu-deprecated.texi |  4 ++++
 qemu-options.hx      | 18 ++----------------
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/qapi/net.json b/qapi/net.json
index 5f7bff1637..59d79a1ae1 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -221,7 +221,8 @@
 #
 # @br: bridge name (since 2.8)
 #
-# @helper: command to execute to configure bridge
+# @helper: command to execute to configure bridge (deprecated, use
+# type 'bridge' instead)
 #
 # @sndbuf: send buffer limit. Understands [TGMKkb] suffixes.
 #
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 50292d820b..52e7600ebc 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -48,6 +48,10 @@ for these file types is 'host_cdrom' or 'host_device' as appropriate.
 The @option{name} parameter of the @option{-net} option is a synonym
 for the @option{id} parameter, which should now be used instead.
 
+@subsection -netdev tap,helper=... (since 4.1)
+
+Use -netdev bridge instead.
+
 @subsection -smp (invalid topologies) (since 3.1)
 
 CPU topology properties should describe whole machine topology including
diff --git a/qemu-options.hx b/qemu-options.hx
index 39dc170429..3324203b51 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2121,7 +2121,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "                configure a host TAP network backend with ID 'str'\n"
 #else
     "-netdev tap,id=str[,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile]\n"
-    "         [,br=bridge][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
+    "         [,br=bridge][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
     "         [,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n"
     "         [,poll-us=n]\n"
     "                configure a host TAP network backend with ID 'str'\n"
@@ -2130,8 +2130,6 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "                to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
     "                to deconfigure it\n"
     "                use '[down]script=no' to disable script execution\n"
-    "                use network helper 'helper' (default=" DEFAULT_BRIDGE_HELPER ") to\n"
-    "                configure it\n"
     "                use 'fd=h' to connect to an already opened TAP interface\n"
     "                use 'fds=x:y:...:z' to connect to already opened multiqueue capable TAP interfaces\n"
     "                use 'sndbuf=nbytes' to limit the size of the send buffer (the\n"
@@ -2435,7 +2433,7 @@ qemu-system-i386 -nic  'user,id=n1,guestfwd=tcp:10.0.2.100:1234-cmd:netcat 10.10
 
 @end table
 
-@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}]
+@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}]
 Configure a host TAP network backend with ID @var{id}.
 
 Use the network script @var{file} to configure it and the network script
@@ -2445,11 +2443,6 @@ automatically provides one. The default network configure script is
 @file{/etc/qemu-ifdown}. Use @option{script=no} or @option{downscript=no}
 to disable script execution.
 
-If running QEMU as an unprivileged user, use the network helper
-@var{helper} to configure the TAP interface and attach it to the bridge.
-The default network helper executable is @file{/path/to/qemu-bridge-helper}
-and the default bridge device is @file{br0}.
-
 @option{fd}=@var{h} can be used to specify the handle of an already
 opened host TAP interface.
 
@@ -2468,13 +2461,6 @@ qemu-system-i386 linux.img \
         -netdev tap,id=nd1,ifname=tap1 -device rtl8139,netdev=nd1
 @end example
 
-@example
-#launch a QEMU instance with the default network helper to
-#connect a TAP device to bridge br0
-qemu-system-i386 linux.img -device virtio-net-pci,netdev=n1 \
-        -netdev tap,id=n1,"helper=/path/to/qemu-bridge-helper"
-@end example
-
 @item -netdev bridge,id=@var{id}[,br=@var{bridge}][,helper=@var{helper}]
 Connect a host TAP network interface to a host bridge device.
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH 2/3] net: Deprecate tap backend's parameter "helper"

[Jason Wang]

On 2019/6/4 下午7:52, Markus Armbruster wrote:
> -netdev tap,helper=... is a useless duplicate of -netdev bridge.
> Deprecate and de-document.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


This requires more thought as TAP could be used independently. Force 
using a "bridge" backend may lead some confusion.

Thanks


> ---
>   qapi/net.json        |  3 ++-
>   qemu-deprecated.texi |  4 ++++
>   qemu-options.hx      | 18 ++----------------
>   3 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 5f7bff1637..59d79a1ae1 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -221,7 +221,8 @@
>   #
>   # @br: bridge name (since 2.8)
>   #
> -# @helper: command to execute to configure bridge
> +# @helper: command to execute to configure bridge (deprecated, use
> +# type 'bridge' instead)
>   #
>   # @sndbuf: send buffer limit. Understands [TGMKkb] suffixes.
>   #
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 50292d820b..52e7600ebc 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -48,6 +48,10 @@ for these file types is 'host_cdrom' or 'host_device' as appropriate.
>   The @option{name} parameter of the @option{-net} option is a synonym
>   for the @option{id} parameter, which should now be used instead.
>   
> +@subsection -netdev tap,helper=... (since 4.1)
> +
> +Use -netdev bridge instead.
> +
>   @subsection -smp (invalid topologies) (since 3.1)
>   
>   CPU topology properties should describe whole machine topology including
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 39dc170429..3324203b51 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2121,7 +2121,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>       "                configure a host TAP network backend with ID 'str'\n"
>   #else
>       "-netdev tap,id=str[,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile]\n"
> -    "         [,br=bridge][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
> +    "         [,br=bridge][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
>       "         [,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n"
>       "         [,poll-us=n]\n"
>       "                configure a host TAP network backend with ID 'str'\n"
> @@ -2130,8 +2130,6 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>       "                to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
>       "                to deconfigure it\n"
>       "                use '[down]script=no' to disable script execution\n"
> -    "                use network helper 'helper' (default=" DEFAULT_BRIDGE_HELPER ") to\n"
> -    "                configure it\n"
>       "                use 'fd=h' to connect to an already opened TAP interface\n"
>       "                use 'fds=x:y:...:z' to connect to already opened multiqueue capable TAP interfaces\n"
>       "                use 'sndbuf=nbytes' to limit the size of the send buffer (the\n"
> @@ -2435,7 +2433,7 @@ qemu-system-i386 -nic  'user,id=n1,guestfwd=tcp:10.0.2.100:1234-cmd:netcat 10.10
>   
>   @end table
>   
> -@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}]
> +@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}]
>   Configure a host TAP network backend with ID @var{id}.
>   
>   Use the network script @var{file} to configure it and the network script
> @@ -2445,11 +2443,6 @@ automatically provides one. The default network configure script is
>   @file{/etc/qemu-ifdown}. Use @option{script=no} or @option{downscript=no}
>   to disable script execution.
>   
> -If running QEMU as an unprivileged user, use the network helper
> -@var{helper} to configure the TAP interface and attach it to the bridge.
> -The default network helper executable is @file{/path/to/qemu-bridge-helper}
> -and the default bridge device is @file{br0}.
> -
>   @option{fd}=@var{h} can be used to specify the handle of an already
>   opened host TAP interface.
>   
> @@ -2468,13 +2461,6 @@ qemu-system-i386 linux.img \
>           -netdev tap,id=nd1,ifname=tap1 -device rtl8139,netdev=nd1
>   @end example
>   
> -@example
> -#launch a QEMU instance with the default network helper to
> -#connect a TAP device to bridge br0
> -qemu-system-i386 linux.img -device virtio-net-pci,netdev=n1 \
> -        -netdev tap,id=n1,"helper=/path/to/qemu-bridge-helper"
> -@end example
> -
>   @item -netdev bridge,id=@var{id}[,br=@var{bridge}][,helper=@var{helper}]
>   Connect a host TAP network interface to a host bridge device.
>   

Re: [Qemu-devel] [PATCH 2/3] net: Deprecate tap backend's parameter "helper"

[Markus Armbruster]

Jason Wang <jasowang@redhat.com> writes:

> On 2019/6/4 下午7:52, Markus Armbruster wrote:
>> -netdev tap,helper=... is a useless duplicate of -netdev bridge.
>> Deprecate and de-document.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>
> This requires more thought as TAP could be used independently. Force
> using a "bridge" backend may lead some confusion.

Can you explain your qualms in a bit more detail?

The thoughts that led to this patch:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg03164.html

Consensus back then:

1. Add qemu-bridge-helper.c to Jason's "Network device backends"

2. Deprecate -netdev tap parameter "helper"

3. Improve documentation of -netdev bridge

4. Create a manual page for qemu-bridge-helper that also covers
   /etc/qemu/bridge.conf.

5. Fix the nutty error handling in parse_acl_file()

This series covers the first two [PATCH 1+2], and records the remaining
three more permanently [PATCH 3].

Re: [Qemu-devel] [PATCH 2/3] net: Deprecate tap backend's parameter "helper"

[Jason Wang]

On 2019/6/18 下午1:32, Markus Armbruster wrote:
> Jason Wang <jasowang@redhat.com> writes:
>
>> On 2019/6/4 下午7:52, Markus Armbruster wrote:
>>> -netdev tap,helper=... is a useless duplicate of -netdev bridge.
>>> Deprecate and de-document.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> This requires more thought as TAP could be used independently. Force
>> using a "bridge" backend may lead some confusion.
> Can you explain your qualms in a bit more detail?
>
> The thoughts that led to this patch:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg03164.html
>
> Consensus back then:
>
> 1. Add qemu-bridge-helper.c to Jason's "Network device backends"
>
> 2. Deprecate -netdev tap parameter "helper"


The problem comes from this point. The main reason is TAP could be used 
without bridge e.g:

- you can simply assign an IP and and properly configure route table on 
host to make it work

- or setup tc actions or using XDP to transfer packets between TAP and 
another interfaces

- using AF_PACKET or other socket to capture the traffic and do the 
forwarding in userspace

So it looks to me switching to use -netdev bridge is inappropriate.

Thanks



>
> 3. Improve documentation of -netdev bridge
>
> 4. Create a manual page for qemu-bridge-helper that also covers
>     /etc/qemu/bridge.conf.
>
> 5. Fix the nutty error handling in parse_acl_file()
>
> This series covers the first two [PATCH 1+2], and records the remaining
> three more permanently [PATCH 3].
>

[Qemu-devel] [PATCH 3/3] qemu-bridge-helper: Document known shortcomings

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-bridge-helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index f9940deefd..95624bc300 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -10,7 +10,17 @@
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
- *
+ */
+
+/*
+ * Known shortcomings:
+ * - There is no manual page
+ * - The syntax of the ACL file is not documented anywhere
+ * - parse_acl_file() doesn't report fopen() failure properly, fails
+ *   to check ferror() after fgets() failure, arbitrarily truncates
+ *   long lines, handles whitespace inconsistently, error messages
+ *   don't point to the offending file and line, errors in included
+ *   files are reported, but otherwise ignored, ...
  */
 
 #include "qemu/osdep.h"
-- 
2.21.0

Re: [Qemu-devel] [PATCH 0/3] Some qemu-bridge-helper work

[Jason Wang]

On 2019/6/4 下午7:52, Markus Armbruster wrote:
> Markus Armbruster (3):
>    MAINTAINERS: Add qemu-bridge-helper.c to "Network device backends"
>    net: Deprecate tap backend's parameter "helper"
>    qemu-bridge-helper: Document known shortcomings
>
>   MAINTAINERS          |  1 +
>   qapi/net.json        |  3 ++-
>   qemu-bridge-helper.c | 12 +++++++++++-
>   qemu-deprecated.texi |  4 ++++
>   qemu-options.hx      | 18 ++----------------
>   5 files changed, 20 insertions(+), 18 deletions(-)
>

I've queued patch 1 and 3. For patch 2, it still require more thought 
since tap is not tied to bridge in fact, it could be used independently.

Thanks