> This looks more or less good-to-go to me. Â Thank you!
>
> (How much have you tested it? Â What kinds of tests did you do?)
I mostly did basic functional testing. Created a bunch of ports, destroyed
them. Created bridges named for ports on other bridges. All the testing I did
used valgrind to look for leaks. I also tested the conf.db which caused
vswitchd to infinite loop to verify that it doesn't.
> The ofpp_garbage list could just be a dynamic array of "uint16_t"s,
> although the current representation is OK too.
I thought about that when implementing it. I think lists are a bit easier to
deal with, at this point I'd rather leave it cause I want to get this in.
> It would work out more cleanly if we changed the "no port yet"
> ofp_port parameter value to be OFPP_NONE, or if we changed the
> semantics of the ofproto_port_add() output parameter to use -1. Â If
> you don't make this change, though, please do notice that both the
> function parameter and the inner block variable are named 'ofp_port'
> currently.
I find the current code a bit easier to reason about as all of the iface
initialization is done in one place. I changed the name in the block to
new_ofp_port.
The following is an incremental:
---
vswitchd/bridge.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 8bf706d..cd10046 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -493,9 +493,6 @@ bridge_reconfigure_ofp(void)
HMAP_FOR_EACH_SAFE (if_cfg, next, hmap_node, &br->if_cfg_todo) {
iface_create(br, if_cfg, -1);
- hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node);
- free(if_cfg);
-
time_refresh();
if (time_msec() >= deadline) {
return false;
@@ -1201,8 +1198,6 @@ bridge_refresh_ofp_port(struct bridge *br)
if (if_cfg) {
iface_create(br, if_cfg, ofproto_port.ofp_port);
- hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node);
- free(if_cfg);
} else if (bridge_has_bond_fake_iface(br, ofproto_port.name)
&& strcmp(ofproto_port.type, "internal")) {
/* Bond fake iface with the wrong type. */
@@ -1234,10 +1229,10 @@ bridge_refresh_ofp_port(struct bridge *br)
}
}
-/* Initialize 'iface' with a netdev and ofp_port if necessary. */
/* Creates a new iface on 'br' based on 'if_cfg'. The new iface has OpenFlow
* port number 'ofp_port'. If ofp_port is negative, an OpenFlow port is
- * automatically allocated for the iface. */
+ * automatically allocated for the iface. Takes ownership of and
+ * deallocates 'if_cfg'. */
static void
iface_create(struct bridge *br, struct if_cfg *if_cfg, int ofp_port)
{
@@ -1268,6 +1263,9 @@ iface_create(struct bridge *br, struct if_cfg *if_cfg,
int ofp_port)
iface_set_ofp_port(iface, ofp_port);
}
+ hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node);
+ free(if_cfg);
+
error = netdev_open(iface->name, iface->type, &iface->netdev);
if (error) {
VLOG_WARN("could not open network device %s (%s)", iface->name,
@@ -1301,14 +1299,14 @@ iface_create(struct bridge *br, struct if_cfg *if_cfg,
int ofp_port)
/* Add the port, if necessary. */
if (iface->netdev && iface->ofp_port < 0) {
- uint16_t ofp_port;
+ uint16_t new_ofp_port;
int error;
- error = ofproto_port_add(br->ofproto, iface->netdev, &ofp_port);
+ error = ofproto_port_add(br->ofproto, iface->netdev, &new_ofp_port);
if (!error) {
VLOG_INFO("bridge %s: added interface %s (%d)", br->name,
iface->name, ofp_port);
- iface_set_ofp_port(iface, ofp_port);
+ iface_set_ofp_port(iface, new_ofp_port);
} else {
netdev_close(iface->netdev);
iface->netdev = NULL;
@@ -2039,8 +2037,8 @@ bridge_run(void)
}
if (!reconfiguring) {
- /* If VLAN splinters are in use, then we need to reconfigure if VLAN
usage
- * has changed. */
+ /* If VLAN splinters are in use, then we need to reconfigure if VLAN
+ * usage has changed. */
vlan_splinters_changed = false;
if (vlan_splinters_enabled_anywhere) {
HMAP_FOR_EACH (br, node, &all_bridges) {
@@ -2065,11 +2063,10 @@ bridge_run(void)
}
if (reconfiguring) {
- if (!reconf_txn) {
- reconf_txn = ovsdb_idl_txn_create(idl);
- }
-
if (cfg) {
+ if (!reconf_txn) {
+ reconf_txn = ovsdb_idl_txn_create(idl);
+ }
if (bridge_reconfigure_continue(cfg)) {
ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg);
}
@@ -2496,7 +2493,6 @@ bridge_add_del_ports(struct bridge *br,
}
}
-
SHASH_FOR_EACH (port_node, &new_ports) {
const struct ovsrec_port *port = port_node->data;
size_t i;
--
1.7.9.6
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
