ovs-vsctl: Prevent creating duplicate VLAN bridges.

ovs-vsctl has the concept of a VLAN (or "fake") bridge, which is a
sort of a sub-bridge that receives only packets on a particular VLAN.
There is no way to distinguish two VLAN bridges with the same parent on the
same VLAN, but until now ovs-vsctl did not prevent creating duplicates or
report them.  This commit fixes the problem.

Reported-by: rwxybh
Reported-at: https://github.com/openvswitch/ovs/issues/21
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Thomas Graf <tgraf@noironetworks.com>

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index efe7817..f6e6994 100644 (file)
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -483,6 +483,9 @@ AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br xapi1 xxx $1])], [1], [],
 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 $1
 ], [OVS_VSCTL_CLEANUP])
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-br dup xenbr0 $1])], [1], [],
+  [ovs-vsctl: bridge xenbr0 already has a child VLAN bridge xapi1 on VLAN $1
+], [OVS_VSCTL_CLEANUP])
 CHECK_BRIDGES([xapi1, xenbr0, $1], [xenbr0, xenbr0, 0])
 CHECK_PORTS([xenbr0], [eth0])
 CHECK_IFACES([xenbr0], [eth0])

diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 1c6334a..bb030e4 100644 (file)
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -192,7 +192,8 @@ 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 0 and 4095.  Initially
+\fIvlan\fR, which must be an integer between 0 and 4095.  The parent
+bridge must not already have a fake bridge for \fIvlan\fR.  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 8ac6d10..e33d79d 100644 (file)
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -814,6 +814,9 @@ struct vsctl_iface {
     struct vsctl_port *port;
 };
 
+static struct vsctl_bridge *find_vlan_bridge(struct vsctl_bridge *parent,
+                                             int vlan);
+
 static char *
 vsctl_context_to_string(const struct vsctl_context *ctx)
 {
@@ -870,7 +873,15 @@ add_bridge_to_cache(struct vsctl_context *ctx,
     br->vlan = vlan;
     hmap_init(&br->children);
     if (parent) {
-        hmap_insert(&parent->children, &br->children_node, hash_int(vlan, 0));
+        struct vsctl_bridge *conflict = find_vlan_bridge(parent, vlan);
+        if (conflict) {
+            VLOG_WARN("%s: bridge has multiple VLAN bridges (%s and %s) "
+                      "for VLAN %d, but only one is allowed",
+                      parent->name, name, conflict->name, vlan);
+        } else {
+            hmap_insert(&parent->children, &br->children_node,
+                        hash_int(vlan, 0));
+        }
     }
     shash_add(&ctx->bridges, br->name, br);
     return br;
@@ -1660,6 +1671,7 @@ cmd_add_br(struct vsctl_context *ctx)
 
         ovs_insert_bridge(ctx->ovs, br);
     } else {
+        struct vsctl_bridge *conflict;
         struct vsctl_bridge *parent;
         struct ovsrec_port *port;
         struct ovsrec_bridge *br;
@@ -1672,6 +1684,11 @@ cmd_add_br(struct vsctl_context *ctx)
         if (!parent) {
             vsctl_fatal("parent bridge %s does not exist", parent_name);
         }
+        conflict = find_vlan_bridge(parent, vlan);
+        if (conflict) {
+            vsctl_fatal("bridge %s already has a child VLAN bridge %s "
+                        "on VLAN %d", parent_name, conflict->name, vlan);
+        }
         br = parent->br_cfg;
 
         iface = ovsrec_interface_insert(ctx->txn);