[ovs-dev] [PATCH] ovs-vsctl: Allow "fake bridges" to be created for VLAN 0.
Justin Pettit
jpettit at nicira.com
Tue Mar 20 18:01:11 UTC 2012
Looks good to me.
Unrelated to this patch, we've still got some inconsistencies about the range of VLANs supported. For example, mirroring and CFM don't allow configuring an output VLAN of zero. Should we look into adding that?
--Justin
On Mar 16, 2012, at 1:13 PM, Ben Pfaff wrote:
> A fake bridge for VLAN 0 is useful, because it provides a way to create
> access ports for VLAN 0. There is no good reason to prevent it.
>
> NIC-464.
> Reported-by: Rob Hoes <Rob.Hoes at citrix.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> tests/ovs-vsctl.at | 64 ++++++++++++++++++++++++++--------------------
> utilities/ovs-vsctl.8.in | 5 ++-
> utilities/ovs-vsctl.c | 21 +++++++++------
> 3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 9d742cc..4b17b05 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -401,8 +401,7 @@ OVS_VSCTL_CLEANUP
> AT_CLEANUP
>
> dnl ----------------------------------------------------------------------
> -AT_BANNER([ovs-vsctl unit tests -- fake bridges])
> -
> +dnl OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([VLAN])
> m4_define([OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF],
> [AT_CHECK(
> [RUN_OVS_VSCTL(
> @@ -410,36 +409,40 @@ m4_define([OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF],
> [--may-exist add-br xenbr0],
> [add-port xenbr0 eth0],
> [--may-exist add-port xenbr0 eth0],
> - [add-br xapi1 xenbr0 9],
> - [--may-exist add-br xapi1 xenbr0 9],
> - [add-port xapi1 eth0.9])],
> + [add-br xapi1 xenbr0 $1],
> + [--may-exist add-br xapi1 xenbr0 $1],
> + [add-port xapi1 eth0.$1])],
> [0], [], [], [OVS_VSCTL_CLEANUP])])
>
> -AT_SETUP([simple fake bridge])
> +dnl OVS_VSCTL_FAKE_BRIDGE_TESTS([VLAN])
> +m4_define([OVS_VSCTL_FAKE_BRIDGE_TESTS], [
> +AT_BANNER([ovs-vsctl unit tests -- fake bridges (VLAN $1)])
> +
> +AT_SETUP([simple fake bridge (VLAN $1)])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF
> +OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([$1])
> AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br xapi1])], [1], [],
> - [ovs-vsctl: "--may-exist add-br xapi1" but xapi1 is a VLAN bridge for VLAN 9
> + [ovs-vsctl: "--may-exist add-br xapi1" but xapi1 is a VLAN bridge for VLAN $1
> ], [OVS_VSCTL_CLEANUP])
> -AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br xapi1 xxx 9])], [1], [],
> - [ovs-vsctl: "--may-exist add-br xapi1 xxx 9" but xapi1 has the wrong parent xenbr0
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br xapi1 xxx $1])], [1], [],
> + [ovs-vsctl: "--may-exist add-br xapi1 xxx $1" but xapi1 has the wrong parent xenbr0
> ], [OVS_VSCTL_CLEANUP])
> AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br xapi1 xenbr0 10])], [1], [],
> - [ovs-vsctl: "--may-exist add-br xapi1 xenbr0 10" but xapi1 is a VLAN bridge for the wrong VLAN 9
> + [ovs-vsctl: "--may-exist add-br xapi1 xenbr0 10" but xapi1 is a VLAN bridge for the wrong VLAN $1
> ], [OVS_VSCTL_CLEANUP])
> -CHECK_BRIDGES([xapi1, xenbr0, 9], [xenbr0, xenbr0, 0])
> +CHECK_BRIDGES([xapi1, xenbr0, $1], [xenbr0, xenbr0, 0])
> CHECK_PORTS([xenbr0], [eth0])
> CHECK_IFACES([xenbr0], [eth0])
> -CHECK_PORTS([xapi1], [eth0.9])
> -CHECK_IFACES([xapi1], [eth0.9])
> +CHECK_PORTS([xapi1], [eth0.$1])
> +CHECK_IFACES([xapi1], [eth0.$1])
> OVS_VSCTL_CLEANUP
> AT_CLEANUP
>
> -AT_SETUP([simple fake bridge + del-br fake bridge])
> +AT_SETUP([simple fake bridge + del-br fake bridge (VLAN $1)])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF
> +OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([$1])
> AT_CHECK([RUN_OVS_VSCTL([del-br xapi1])], [0], [], [], [OVS_VSCTL_CLEANUP])
> CHECK_BRIDGES([xenbr0, xenbr0, 0])
> CHECK_PORTS([xenbr0], [eth0])
> @@ -447,19 +450,19 @@ CHECK_IFACES([xenbr0], [eth0])
> OVS_VSCTL_CLEANUP
> AT_CLEANUP
>
> -AT_SETUP([simple fake bridge + del-br real bridge])
> +AT_SETUP([simple fake bridge + del-br real bridge (VLAN $1)])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF
> +OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([$1])
> AT_CHECK([RUN_OVS_VSCTL([del-br xenbr0])], [0], [], [], [OVS_VSCTL_CLEANUP])
> CHECK_BRIDGES
> OVS_VSCTL_CLEANUP
> AT_CLEANUP
>
> -AT_SETUP([simple fake bridge + external IDs])
> +AT_SETUP([simple fake bridge + external IDs (VLAN $1)])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF
> +OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([$1])
> AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
> [br-set-external-id xenbr0 key0 value0],
> [br-set-external-id xapi1 key1 value1],
> @@ -473,27 +476,32 @@ value0
> key1=value1
> value1
> ], [], [OVS_VSCTL_CLEANUP])
> -CHECK_BRIDGES([xapi1, xenbr0, 9], [xenbr0, xenbr0, 0])
> +CHECK_BRIDGES([xapi1, xenbr0, $1], [xenbr0, xenbr0, 0])
> CHECK_PORTS([xenbr0], [eth0])
> CHECK_IFACES([xenbr0], [eth0])
> -CHECK_PORTS([xapi1], [eth0.9])
> -CHECK_IFACES([xapi1], [eth0.9])
> +CHECK_PORTS([xapi1], [eth0.$1])
> +CHECK_IFACES([xapi1], [eth0.$1])
> OVS_VSCTL_CLEANUP
> AT_CLEANUP
> +]) # OVS_VSCTL_FAKE_BRIDGE_TESTS
> +
> +OVS_VSCTL_FAKE_BRIDGE_TESTS([9])
> +OVS_VSCTL_FAKE_BRIDGE_TESTS([0])
>
> +dnl OVS_VSCTL_SETUP_BOND_FAKE_CONF([VLAN])
> m4_define([OVS_VSCTL_SETUP_BOND_FAKE_CONF],
> [AT_CHECK(
> [RUN_OVS_VSCTL(
> [add-br xapi1],
> [add-bond xapi1 bond0 eth0 eth1],
> - [add-br xapi2 xapi1 11],
> - [add-port xapi2 bond0.11])],
> + [add-br xapi2 xapi1 $1],
> + [add-port xapi2 bond0.$1])],
> [0], [], [], [OVS_VSCTL_CLEANUP])])
>
> AT_SETUP([fake bridge on bond])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_BOND_FAKE_CONF
> +OVS_VSCTL_SETUP_BOND_FAKE_CONF([11])
> CHECK_BRIDGES([xapi1, xapi1, 0], [xapi2, xapi1, 11])
> CHECK_PORTS([xapi1], [bond0])
> CHECK_IFACES([xapi1], [eth0], [eth1])
> @@ -505,7 +513,7 @@ AT_CLEANUP
> AT_SETUP([fake bridge on bond + del-br fake bridge])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_BOND_FAKE_CONF
> +OVS_VSCTL_SETUP_BOND_FAKE_CONF([11])
> AT_CHECK([RUN_OVS_VSCTL_ONELINE([del-br xapi2])], [0], [
> ], [], [OVS_VSCTL_CLEANUP])
> CHECK_BRIDGES([xapi1, xapi1, 0])
> @@ -517,7 +525,7 @@ AT_CLEANUP
> AT_SETUP([fake bridge on bond + del-br real bridge])
> AT_KEYWORDS([ovs-vsctl fake-bridge])
> OVS_VSCTL_SETUP
> -OVS_VSCTL_SETUP_BOND_FAKE_CONF
> +OVS_VSCTL_SETUP_BOND_FAKE_CONF([11])
> AT_CHECK([RUN_OVS_VSCTL([del-br xapi1])])
> CHECK_BRIDGES
> OVS_VSCTL_CLEANUP
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 0d858a1..57f76d5 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -69,7 +69,8 @@ When such a ``fake bridge'' is active, \fBovs\-vsctl\fR will treat it
> much like a bridge separate from its ``parent bridge,'' but the actual
> implementation in Open vSwitch uses only a single bridge, with ports on
> the fake bridge assigned the implicit VLAN of the fake bridge of which
> -they are members.
> +they are members. (A fake bridge for VLAN 0 receives packets that
> +have no 802.1Q tag or a tag with VLAN 0.)
> .
> .SH OPTIONS
> .
> @@ -179,7 +180,7 @@ nothing if \fIbridge\fR already exists as a real bridge.
> Creates a ``fake bridge'' named \fIbridge\fR within the existing Open
> vSwitch bridge \fIparent\fR, which must already exist and must not
> itself be a fake bridge. The new fake bridge will be on 802.1Q VLAN
> -\fIvlan\fR, which must be an integer between 1 and 4095. Initially
> +\fIvlan\fR, which must be an integer between 0 and 4095. Initially
> \fIbridge\fR will have no ports (other than \fIbridge\fR itself).
> .IP
> Without \fB\-\-may\-exist\fR, attempting to create a bridge that
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index db4e910..b278f70 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -612,8 +612,13 @@ struct vsctl_bridge {
> struct ovsrec_controller **ctrl;
> char *fail_mode;
> size_t n_ctrl;
> - struct vsctl_bridge *parent;
> - int vlan;
> +
> + /* VLAN ("fake") bridge support.
> + *
> + * Use 'parent != NULL' to detect a fake bridge, because 'vlan' can be 0
> + * in either case. */
> + struct vsctl_bridge *parent; /* Real bridge, or NULL. */
> + int vlan; /* VLAN VID (0...4095), or 0. */
> };
>
> struct vsctl_port {
> @@ -704,7 +709,7 @@ port_is_fake_bridge(const struct ovsrec_port *port_cfg)
> {
> return (port_cfg->fake_bridge
> && port_cfg->tag
> - && *port_cfg->tag >= 1 && *port_cfg->tag <= 4095);
> + && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095);
> }
>
> static struct vsctl_bridge *
> @@ -841,7 +846,7 @@ get_info(struct vsctl_context *ctx, struct vsctl_info *info)
> port = xmalloc(sizeof *port);
> port->port_cfg = port_cfg;
> if (port_cfg->tag
> - && *port_cfg->tag >= 1 && *port_cfg->tag <= 4095) {
> + && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) {
> port->bridge = find_vlan_bridge(info, br, *port_cfg->tag);
> if (!port->bridge) {
> port->bridge = br;
> @@ -1329,8 +1334,8 @@ cmd_add_br(struct vsctl_context *ctx)
> } else if (ctx->argc == 4) {
> parent_name = ctx->argv[2];
> vlan = atoi(ctx->argv[3]);
> - if (vlan < 1 || vlan > 4095) {
> - vsctl_fatal("%s: vlan must be between 1 and 4095", ctx->argv[0]);
> + if (vlan < 0 || vlan > 4095) {
> + vsctl_fatal("%s: vlan must be between 0 and 4095", ctx->argv[0]);
> }
> } else {
> vsctl_fatal("'%s' command takes exactly 1 or 3 arguments",
> @@ -1397,7 +1402,7 @@ cmd_add_br(struct vsctl_context *ctx)
> int64_t tag = vlan;
>
> parent = find_bridge(&info, parent_name, false);
> - if (parent && parent->vlan) {
> + if (parent && parent->parent) {
> vsctl_fatal("cannot create bridge with fake bridge as parent");
> }
> if (!parent) {
> @@ -1750,7 +1755,7 @@ add_port(struct vsctl_context *ctx,
> ovsrec_port_set_bond_fake_iface(port, fake_iface);
> free(ifaces);
>
> - if (bridge->vlan) {
> + if (bridge->parent) {
> int64_t tag = bridge->vlan;
> ovsrec_port_set_tag(port, &tag, 1);
> }
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list