> 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?
The CFM code assumes that having a zero vlan and no vlan tag are
equivalent. This difference is important because if you configure a
cfm_ccm_pcp value, and don't configure a vlan, you'll get a vlan_tci
on your ethernet frame with a 0 vlan and whatever pcp you configured.
If we want users to be able to force the CFM module to add a zero
vlan tag, it would be pretty simple. Probably the best way to do it
is add a CFM_ZERO_VLAN #define similar to the CFM_RANDOM_VLAN which
was recently added. Is this actually useful? Do real networks treat
0 vlan_tci differently from an absent vlan_tci?
One other thing I've been thinking about: the way you configure a
VLAN on CFM is quite a bit different then how it's configured for the
rest of the Interface (i.e. it has it's own vlan configuration instead
of using the vlan configuration in the port table). It seems to me
that the CFM module should use whatever vlan is configured in the port
table, and only accept CCMs from VLANs configured as trunks. Of
course, the cfm_ccm_vlan configuration option would still be available
to override on a per-interface basis. This is probably the right
thing to do in the medium term, not sure how important it is to get
down quickly though.
Ethan
>
> --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.h...@citrix.com>
>> Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
