Message ID | 1473750714-48290-1-git-send-email-aik@ozlabs.ru |
---|---|
State | New |
Headers | show |
On 09/13/2016 02:11 AM, Alexey Kardashevskiy wrote: > The tap backend is already using qemu-bridge-helper to attach tap > interface to a bridge but (unlike the bridge backend) it always uses > the default bridge name - br0. > > This adds a "br" property support to the tap backend. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > -- > Changes: > v2: > * documented a new member in json and hx > --- > net/tap.c | 4 +++- > qapi-schema.json | 3 +++ > qemu-options.hx | 12 +++++++----- > 3 files changed, 13 insertions(+), 6 deletions(-) > > +++ b/qapi-schema.json > @@ -2575,6 +2575,8 @@ > # > # @downscript: #optional script to shut down the interface > # > +# @br: #optional bridge name Missing a '(since 2.8)' designator. Also, we don't have to abbreviate; 'bridge-name' may be easier to understand than 'br', as well as a mention of the default value if the parameter is not supplied. > +# > # @helper: #optional command to execute to configure bridge > # > # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes. > @@ -2604,6 +2606,7 @@ > '*fds': 'str', > '*script': 'str', > '*downscript': 'str', > + '*br': 'str', > '*helper': 'str', > '*sndbuf': 'size', > '*vnet_hdr': 'bool', Oh, we already use underscore, so if you go with a longer name, 'bridge_name' would be more consistent than 'bridge-name', even though we prefer dash over underscore in new interfaces.
On Tue, 13 Sep 2016 09:49:09 -0500 Eric Blake <eblake@redhat.com> wrote: > On 09/13/2016 02:11 AM, Alexey Kardashevskiy wrote: > > The tap backend is already using qemu-bridge-helper to attach tap > > interface to a bridge but (unlike the bridge backend) it always uses > > the default bridge name - br0. > > > > This adds a "br" property support to the tap backend. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > -- > > Changes: > > v2: > > * documented a new member in json and hx > > --- > > net/tap.c | 4 +++- > > qapi-schema.json | 3 +++ > > qemu-options.hx | 12 +++++++----- > > 3 files changed, 13 insertions(+), 6 deletions(-) > > > > > +++ b/qapi-schema.json > > @@ -2575,6 +2575,8 @@ > > # > > # @downscript: #optional script to shut down the interface > > # > > +# @br: #optional bridge name > > Missing a '(since 2.8)' designator. > > Also, we don't have to abbreviate; 'bridge-name' may be easier to > understand than 'br', as well as a mention of the default value if the > parameter is not supplied. > FWIW @br is consistent with what we already have in NetdevBridgeOptions since 1.2. > > +# > > # @helper: #optional command to execute to configure bridge > > # > > # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes. > > @@ -2604,6 +2606,7 @@ > > '*fds': 'str', > > '*script': 'str', > > '*downscript': 'str', > > + '*br': 'str', > > '*helper': 'str', > > '*sndbuf': 'size', > > '*vnet_hdr': 'bool', > > Oh, we already use underscore, so if you go with a longer name, > 'bridge_name' would be more consistent than 'bridge-name', even though > we prefer dash over underscore in new interfaces. >
On 14/09/16 01:39, Greg Kurz wrote: > On Tue, 13 Sep 2016 09:49:09 -0500 > Eric Blake <eblake@redhat.com> wrote: > >> On 09/13/2016 02:11 AM, Alexey Kardashevskiy wrote: >>> The tap backend is already using qemu-bridge-helper to attach tap >>> interface to a bridge but (unlike the bridge backend) it always uses >>> the default bridge name - br0. >>> >>> This adds a "br" property support to the tap backend. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> -- >>> Changes: >>> v2: >>> * documented a new member in json and hx >>> --- >>> net/tap.c | 4 +++- >>> qapi-schema.json | 3 +++ >>> qemu-options.hx | 12 +++++++----- >>> 3 files changed, 13 insertions(+), 6 deletions(-) >>> >> >>> +++ b/qapi-schema.json >>> @@ -2575,6 +2575,8 @@ >>> # >>> # @downscript: #optional script to shut down the interface >>> # >>> +# @br: #optional bridge name >> >> Missing a '(since 2.8)' designator. >> >> Also, we don't have to abbreviate; 'bridge-name' may be easier to >> understand than 'br', as well as a mention of the default value if the >> parameter is not supplied. The existing NetdevBridgeOptions does not do this in json as well and it does in qemu-options.hx, so does my patch. Do I have to fix a bridge first before reposting this patch?... When exactly is the content of qapi-schema.json's comments shown to a user? >> > > FWIW @br is consistent with what we already have in NetdevBridgeOptions > since 1.2. Right, this is why "br=".
On 09/13/2016 11:23 PM, Alexey Kardashevskiy wrote: >>> >>> Also, we don't have to abbreviate; 'bridge-name' may be easier to >>> understand than 'br', as well as a mention of the default value if the >>> parameter is not supplied. > > > The existing NetdevBridgeOptions does not do this in json as well and it > does in qemu-options.hx, so does my patch. Do I have to fix a bridge first > before reposting this patch?... If that interface is already baked in as 'br', then leave it alone. > > When exactly is the content of qapi-schema.json's comments shown to a user? Not yet, but Marc-Andre has patches pending that merge qemu-options.hx and qemu-schema.json so that we have a single file with documentation, rather than two semi-inconsistent copies. > > >>> >> >> FWIW @br is consistent with what we already have in NetdevBridgeOptions >> since 1.2. > > Right, this is why "br=". Okay, the argument of consistency with pre-existing interfaces is reasonable.
On Tue, 13 Sep 2016 17:11:54 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > The tap backend is already using qemu-bridge-helper to attach tap > interface to a bridge but (unlike the bridge backend) it always uses > the default bridge name - br0. > > This adds a "br" property support to the tap backend. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > -- Cool ! This allows to easily attach to virbr0 :) Now that a consensus seems to have been reached for @br: Reviewed-by: Greg Kurz <groug@kaod.org> Tested-by: Greg Kurz <groug@kaod.org> Also Cc'ing the tap device maintainer. Cheers. -- Greg > Changes: > v2: > * documented a new member in json and hx > --- > net/tap.c | 4 +++- > qapi-schema.json | 3 +++ > qemu-options.hx | 12 +++++++----- > 3 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/net/tap.c b/net/tap.c > index 6abb962..b6896a7 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -857,7 +857,9 @@ free_fail: > return -1; > } > > - fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE, > + fd = net_bridge_run_helper(tap->helper, > + tap->has_br ? > + tap->br : DEFAULT_BRIDGE_INTERFACE, > errp); > if (fd == -1) { > return -1; > diff --git a/qapi-schema.json b/qapi-schema.json > index c4f3674..dbb9f8f 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2575,6 +2575,8 @@ > # > # @downscript: #optional script to shut down the interface > # > +# @br: #optional bridge name > +# > # @helper: #optional command to execute to configure bridge > # > # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes. > @@ -2604,6 +2606,7 @@ > '*fds': 'str', > '*script': 'str', > '*downscript': 'str', > + '*br': 'str', > '*helper': 'str', > '*sndbuf': 'size', > '*vnet_hdr': 'bool', > diff --git a/qemu-options.hx b/qemu-options.hx > index a71aaf8..0b3ea42 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1594,10 +1594,11 @@ 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" > - " [,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n" > + " [,br=bridge][,helper=helper][,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" > + " connected to a bridge (default=" DEFAULT_BRIDGE_INTERFACE ")\n" > " use network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT ")\n" > " to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n" > " to deconfigure it\n" > @@ -1884,8 +1885,8 @@ processed and applied to -net user. Mixing them with the new configuration > syntax gives undefined results. Their use for new applications is discouraged > as they will be removed from future versions. > > -@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,helper=@var{helper}] > -@itemx -net tap[,vlan=@var{n}][,name=@var{name}][,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,helper=@var{helper}] > +@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}] > +@itemx -net tap[,vlan=@var{n}][,name=@var{name}][,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}] > Connect the host TAP network interface @var{name} to VLAN @var{n}. > > Use the network script @var{file} to configure it and the network script > @@ -1896,8 +1897,9 @@ automatically provides one. The default network configure script is > to disable script execution. > > If running QEMU as an unprivileged user, use the network helper > -@var{helper} to configure the TAP interface. The default network > -helper executable is @file{/path/to/qemu-bridge-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.
On 09/14/2016 09:17 AM, Greg Kurz wrote: > On Tue, 13 Sep 2016 17:11:54 +1000 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> The tap backend is already using qemu-bridge-helper to attach tap >> interface to a bridge but (unlike the bridge backend) it always uses >> the default bridge name - br0. >> >> This adds a "br" property support to the tap backend. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> -- > > Cool ! This allows to easily attach to virbr0 :) > > Now that a consensus seems to have been reached for @br: Yes, but there was still my other comment that: >> +++ b/qapi-schema.json >> @@ -2575,6 +2575,8 @@ >> # >> # @downscript: #optional script to shut down the interface >> # >> +# @br: #optional bridge name this is missing a '(since 2.8)' designator. Perhaps the maintainer can supply it without needing a v3.
On 13/09/2016 09:11, Alexey Kardashevskiy wrote: > The tap backend is already using qemu-bridge-helper to attach tap > interface to a bridge but (unlike the bridge backend) it always uses > the default bridge name - br0. > > This adds a "br" property support to the tap backend. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Stupid question ahead: how does -netdev bridge compare to -netdev tap after this patch? Is there a case left where you must use -netdev bridge? Or can we make -netdev bridge a synonym for "-netdev tap,helper=/default/path/to/helper"? Thanks, Paolo
On 15/09/16 07:04, Paolo Bonzini wrote: > > > On 13/09/2016 09:11, Alexey Kardashevskiy wrote: >> The tap backend is already using qemu-bridge-helper to attach tap >> interface to a bridge but (unlike the bridge backend) it always uses >> the default bridge name - br0. >> >> This adds a "br" property support to the tap backend. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Stupid question ahead: how does -netdev bridge compare to -netdev tap > after this patch? Is there a case left where you must use -netdev bridge? > > Or can we make -netdev bridge a synonym for "-netdev > tap,helper=/default/path/to/helper"? I looked through history but I could not understand why "bridge" was introduced in the first place as even there (a7c36ee4920ea) is an example of -netdev tap,helper="/usr/local/libexec/qemu-bridge-helper --br=qemubr0",id=hn0 so it was assumed even then that people might want tap on a specific bridge. So my stupid question is - what do I have to do to get this accepted (besides a note that it is 2.8+) or it is not interesting to anyone? :)
On 19/09/2016 02:33, Alexey Kardashevskiy wrote: > On 15/09/16 07:04, Paolo Bonzini wrote: >> >> >> On 13/09/2016 09:11, Alexey Kardashevskiy wrote: >>> The tap backend is already using qemu-bridge-helper to attach tap >>> interface to a bridge but (unlike the bridge backend) it always uses >>> the default bridge name - br0. >>> >>> This adds a "br" property support to the tap backend. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Stupid question ahead: how does -netdev bridge compare to -netdev tap >> after this patch? Is there a case left where you must use -netdev bridge? >> >> Or can we make -netdev bridge a synonym for "-netdev >> tap,helper=/default/path/to/helper"? > > I looked through history but I could not understand why "bridge" was > introduced in the first place as even there (a7c36ee4920ea) is an example of > > -netdev tap,helper="/usr/local/libexec/qemu-bridge-helper --br=qemubr0",id=hn0 > > so it was assumed even then that people might want tap on a specific bridge. > > So my stupid question is - what do I have to do to get this accepted > (besides a note that it is 2.8+) or it is not interesting to anyone? :) I think the patch is even more interesting because it lets us simplify the code for -netdev bridge. Paolo
On 2016年09月14日 23:12, Eric Blake wrote: > On 09/14/2016 09:17 AM, Greg Kurz wrote: >> On Tue, 13 Sep 2016 17:11:54 +1000 >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> >>> The tap backend is already using qemu-bridge-helper to attach tap >>> interface to a bridge but (unlike the bridge backend) it always uses >>> the default bridge name - br0. >>> >>> This adds a "br" property support to the tap backend. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> -- >> Cool ! This allows to easily attach to virbr0 :) >> >> Now that a consensus seems to have been reached for @br: > Yes, but there was still my other comment that: > > >>> +++ b/qapi-schema.json >>> @@ -2575,6 +2575,8 @@ >>> # >>> # @downscript: #optional script to shut down the interface >>> # >>> +# @br: #optional bridge name > this is missing a '(since 2.8)' designator. Perhaps the maintainer can > supply it without needing a v3. > Adding "since 2.8" and applied. Thanks
On 2016年09月19日 19:59, Paolo Bonzini wrote: > > On 19/09/2016 02:33, Alexey Kardashevskiy wrote: >> On 15/09/16 07:04, Paolo Bonzini wrote: >>> >>> On 13/09/2016 09:11, Alexey Kardashevskiy wrote: >>>> The tap backend is already using qemu-bridge-helper to attach tap >>>> interface to a bridge but (unlike the bridge backend) it always uses >>>> the default bridge name - br0. >>>> >>>> This adds a "br" property support to the tap backend. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> Stupid question ahead: how does -netdev bridge compare to -netdev tap >>> after this patch? Is there a case left where you must use -netdev bridge? >>> >>> Or can we make -netdev bridge a synonym for "-netdev >>> tap,helper=/default/path/to/helper"? >> I looked through history but I could not understand why "bridge" was >> introduced in the first place as even there (a7c36ee4920ea) is an example of >> >> -netdev tap,helper="/usr/local/libexec/qemu-bridge-helper --br=qemubr0",id=hn0 >> >> so it was assumed even then that people might want tap on a specific bridge. >> >> So my stupid question is - what do I have to do to get this accepted >> (besides a note that it is 2.8+) or it is not interesting to anyone? :) > I think the patch is even more interesting because it lets us simplify > the code for -netdev bridge. > > Paolo > According to http://wiki.qemu.org/Features-Done/HelperNetworking. Looks like this allows creating a third party backend without modifying qemu. And the bridge helper is an example for this.
diff --git a/net/tap.c b/net/tap.c index 6abb962..b6896a7 100644 --- a/net/tap.c +++ b/net/tap.c @@ -857,7 +857,9 @@ free_fail: return -1; } - fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE, + fd = net_bridge_run_helper(tap->helper, + tap->has_br ? + tap->br : DEFAULT_BRIDGE_INTERFACE, errp); if (fd == -1) { return -1; diff --git a/qapi-schema.json b/qapi-schema.json index c4f3674..dbb9f8f 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2575,6 +2575,8 @@ # # @downscript: #optional script to shut down the interface # +# @br: #optional bridge name +# # @helper: #optional command to execute to configure bridge # # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes. @@ -2604,6 +2606,7 @@ '*fds': 'str', '*script': 'str', '*downscript': 'str', + '*br': 'str', '*helper': 'str', '*sndbuf': 'size', '*vnet_hdr': 'bool', diff --git a/qemu-options.hx b/qemu-options.hx index a71aaf8..0b3ea42 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1594,10 +1594,11 @@ 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" - " [,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n" + " [,br=bridge][,helper=helper][,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" + " connected to a bridge (default=" DEFAULT_BRIDGE_INTERFACE ")\n" " use network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT ")\n" " to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n" " to deconfigure it\n" @@ -1884,8 +1885,8 @@ processed and applied to -net user. Mixing them with the new configuration syntax gives undefined results. Their use for new applications is discouraged as they will be removed from future versions. -@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,helper=@var{helper}] -@itemx -net tap[,vlan=@var{n}][,name=@var{name}][,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,helper=@var{helper}] +@item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}] +@itemx -net tap[,vlan=@var{n}][,name=@var{name}][,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}] Connect the host TAP network interface @var{name} to VLAN @var{n}. Use the network script @var{file} to configure it and the network script @@ -1896,8 +1897,9 @@ automatically provides one. The default network configure script is to disable script execution. If running QEMU as an unprivileged user, use the network helper -@var{helper} to configure the TAP interface. The default network -helper executable is @file{/path/to/qemu-bridge-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.
The tap backend is already using qemu-bridge-helper to attach tap interface to a bridge but (unlike the bridge backend) it always uses the default bridge name - br0. This adds a "br" property support to the tap backend. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> -- Changes: v2: * documented a new member in json and hx --- net/tap.c | 4 +++- qapi-schema.json | 3 +++ qemu-options.hx | 12 +++++++----- 3 files changed, 13 insertions(+), 6 deletions(-)