[ovs-dev] [PATCH] Reimplement port mirroring configuration for database.
Ben Pfaff
blp at nicira.com
Tue Jan 19 18:44:18 UTC 2010
On Tue, Jan 19, 2010 at 11:22:33AM -0500, Jesse Gross wrote:
> Looks good, just a few comments below:
Thanks for the review!
> Did you mean to use the any_ports_specified variable in the test below?
>
> >+ any_ports_specified = cfg->n_select_dst_port || cfg->n_select_dst_port;
> >+ if ((cfg->n_select_dst_port || cfg->n_select_dst_port)
> >+ && shash_is_empty(&src_ports) && shash_is_empty(&dst_ports)) {
Yes. Thanks, I changed "(cfg->n_select_dst_port ||
cfg->n_select_dst_port)" to any_ports_specified.
> >+ VLOG_ERR("bridge %s: disabling mirror %s since none of the specified "
> >+ "selection ports exist", m->bridge->name, m->name);
> > mirror_destroy(m);
> > goto exit;
> > }
>
> If ports are specified but none are valid we destroy the mirror but
> if VLANs are specified but none are valid we let it slide. Not only
> does this seem inconsistent, it could also lead to all VLANs being
> mirrored.
Well, that's what the former code did. The reason was that it is much
easier to get a port name wrong. Ports change over time, after all, but
valid VLANs don't.
But you're right, it's inconsistent. So I applied this incrementally:
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 79622d9..d7f1979 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3677,6 +3677,7 @@ mirror_reconfigure_one(struct mirror *m, struct ovsrec_mirror *cfg)
size_t i;
bool mirror_all_ports;
bool any_ports_specified;
+ bool any_vlans_specified;
/* Get output port. */
if (cfg->output_port) {
@@ -3712,16 +3713,23 @@ mirror_reconfigure_one(struct mirror *m, struct ovsrec_mirror *cfg)
mirror_collect_ports(m, cfg->select_dst_port, cfg->n_select_dst_port,
&dst_ports);
any_ports_specified = cfg->n_select_dst_port || cfg->n_select_dst_port;
- if ((cfg->n_select_dst_port || cfg->n_select_dst_port)
+ if (any_ports_specified
&& shash_is_empty(&src_ports) && shash_is_empty(&dst_ports)) {
VLOG_ERR("bridge %s: disabling mirror %s since none of the specified "
- "selection ports exist", m->bridge->name, m->name);
+ "selection ports exists", m->bridge->name, m->name);
mirror_destroy(m);
goto exit;
}
/* Get all the vlans, and drop duplicate and invalid vlans. */
n_vlans = mirror_collect_vlans(m, cfg, &vlans);
+ any_vlans_specified = cfg->n_select_vlan > 0;
+ if (any_vlans_specified && !n_vlans) {
+ VLOG_ERR("bridge %s: disabling mirror %s since none of the specified "
+ "VLANs exists", m->bridge->name, m->name);
+ mirror_destroy(m);
+ goto exit;
+ }
/* Update mirror data. */
if (!shash_equal_keys(&m->src_ports, &src_ports)
@@ -3741,9 +3749,7 @@ mirror_reconfigure_one(struct mirror *m, struct ovsrec_mirror *cfg)
m->out_vlan = out_vlan;
/* If no selection criteria have been given, mirror for all ports. */
- mirror_all_ports = (shash_is_empty(&m->src_ports)
- && shash_is_empty(&m->dst_ports)
- && !m->n_vlans);
+ mirror_all_ports = !any_ports_specified && !any_vlans_specified;
/* Update ports. */
mirror_bit = MIRROR_MASK_C(1) << m->idx;
> > /* Get all the vlans, and drop duplicate and invalid vlans. */
> >- svec_init(&vlan_strings);
> >- cfg_get_all_keys(&vlan_strings, "%s.select.vlan", pfx);
> >- n_vlans = prune_vlans(m, &vlan_strings, &vlans);
> >- svec_destroy(&vlan_strings);
> >+ n_vlans = mirror_collect_vlans(m, cfg, &vlans);
>
> flood_vlans is probably a better name for this key than
> learning-disable. However, some of the code and error messages
> still refer to learning disabled VLANs. It would be nice to make
> them consistent.
I found a few mentions, so I fixed them in the following additional
commit:
>From 8f30d09ab053824b0ee7297fc8475fc1405beb93 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Tue, 19 Jan 2010 10:41:46 -0800
Subject: [PATCH] mac-learning: Rename "non-learning VLANs" to "flood VLANs".
Usually positive names are better than negative ones.
---
lib/mac-learning.c | 27 +++++++++++++--------------
lib/mac-learning.h | 10 +++++-----
vswitchd/bridge.c | 4 ++--
3 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index 3f1db14..a9d414d 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2010 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -130,7 +130,7 @@ mac_learning_create(void)
list_push_front(&ml->free, &s->lru_node);
}
ml->secret = random_uint32();
- ml->non_learning_vlans = NULL;
+ ml->flood_vlans = NULL;
return ml;
}
@@ -139,24 +139,24 @@ void
mac_learning_destroy(struct mac_learning *ml)
{
if (ml) {
- bitmap_free(ml->non_learning_vlans);
+ bitmap_free(ml->flood_vlans);
}
free(ml);
}
-/* Provides a bitmap of VLANs which have learning disabled. It takes
- * ownership of the bitmap. Returns true if the set has changed from
- * the previous value. */
+/* Provides a bitmap of VLANs which have learning disabled, that is, VLANs on
+ * which all packets are flooded. It takes ownership of the bitmap. Returns
+ * true if the set has changed from the previous value. */
bool
-mac_learning_set_disabled_vlans(struct mac_learning *ml, unsigned long *bitmap)
+mac_learning_set_flood_vlans(struct mac_learning *ml, unsigned long *bitmap)
{
bool ret = (bitmap == NULL
- ? ml->non_learning_vlans != NULL
- : (ml->non_learning_vlans == NULL
- || !bitmap_equal(bitmap, ml->non_learning_vlans, 4096)));
+ ? ml->flood_vlans != NULL
+ : (ml->flood_vlans == NULL
+ || !bitmap_equal(bitmap, ml->flood_vlans, 4096)));
- bitmap_free(ml->non_learning_vlans);
- ml->non_learning_vlans = bitmap;
+ bitmap_free(ml->flood_vlans);
+ ml->flood_vlans = bitmap;
return ret;
}
@@ -164,8 +164,7 @@ mac_learning_set_disabled_vlans(struct mac_learning *ml, unsigned long *bitmap)
static bool
is_learning_vlan(const struct mac_learning *ml, uint16_t vlan)
{
- return !(ml->non_learning_vlans
- && bitmap_is_set(ml->non_learning_vlans, vlan));
+ return !(ml->flood_vlans && bitmap_is_set(ml->flood_vlans, vlan));
}
/* Attempts to make 'ml' learn from the fact that a frame from 'src_mac' was
diff --git a/lib/mac-learning.h b/lib/mac-learning.h
index ed843cd..c4a0e28 100644
--- a/lib/mac-learning.h
+++ b/lib/mac-learning.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2010 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -51,14 +51,14 @@ struct mac_learning {
front, most recently used at the back. */
struct list table[MAC_HASH_SIZE]; /* Hash table. */
struct mac_entry entries[MAC_MAX]; /* All entries. */
- uint32_t secret; /* Secret for */
- unsigned long *non_learning_vlans; /* Bitmap of learning disabled VLANs. */
+ uint32_t secret; /* Secret for randomizing hash table. */
+ unsigned long *flood_vlans; /* Bitmap of learning disabled VLANs. */
};
struct mac_learning *mac_learning_create(void);
void mac_learning_destroy(struct mac_learning *);
-bool mac_learning_set_disabled_vlans(struct mac_learning *,
- unsigned long *bitmap);
+bool mac_learning_set_flood_vlans(struct mac_learning *,
+ unsigned long *bitmap);
tag_type mac_learning_learn(struct mac_learning *,
const uint8_t src[ETH_ADDR_LEN], uint16_t vlan,
uint16_t src_port);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d7f1979..a09f343 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3522,7 +3522,7 @@ mirror_reconfigure(struct bridge *br)
}
}
- /* Update learning disabled vlans (for RSPAN). */
+ /* Update flooded vlans (for RSPAN). */
rspan_vlans = NULL;
if (br->cfg->n_flood_vlans) {
rspan_vlans = bitmap_allocate(4096);
@@ -3539,7 +3539,7 @@ mirror_reconfigure(struct bridge *br)
}
}
}
- if (mac_learning_set_disabled_vlans(br->ml, rspan_vlans)) {
+ if (mac_learning_set_flood_vlans(br->ml, rspan_vlans)) {
bridge_flush(br);
}
}
--
1.6.3.3
> >+ "flood_vlans": {
> >+ "comment": "VLAN IDs of VLANs on which MAC address learning should be disabled, so that packets are flooded instead of being sent to specific ports that are believed to contain packets' destination MACs. This should ordinarily be used to disable MAC learning on VLANs used for mirroring (RSPAN VLANs). It may also be useful for debugging.",
> >+ "type": {"key": "integer", "min": 0, "max": 4096}
Is there something wrong in the snippet above? I didn't see any
commentary on it.
More information about the dev
mailing list