On Sun, May 22, 2016 at 04:36:18PM -0500, Ryan Moats wrote:
> As a side effect, tunnel context is persisted.
>
> Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
Thanks for the new version! I hardly have any comments this time so I
think we're really close ;-)
There appears to be a memory leak in the process_full_encaps case. At
least, I think that this loop should free(old_hash_node):
+ HMAP_FOR_EACH_POP(old_hash_node, node, &keep_tunnel_hmap_by_uuid) {
+ ;
+ }
+ hmap_destroy(&keep_tunnel_hmap_by_uuid);
If it does not need to free the hash node, then I do not think that the
loop does anything useful, because it's OK to destroy an hmap that
still contains nodes.
Please start comments with a capital letter and end them with a period,
e.g. in these cases:
+ /* lookup the tunnel by row uuid and remove it */
+ /* track the southbound idl */
check_and_update_tunnel() tries to use ovsrec_interface_set_name() on an
existing tunnel, but the server will reject this because the 'name'
column in the Interface table is immutable.
When check_and_update_tunnel() fails to find a tunnel, should it try to
create it? Alternatively, should it warn that it does not exist?
Thanks,
Ben.
P.S. Here are some style fixes I suggest folding in:
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 226d0a1..7fa55ed 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -129,7 +129,7 @@ port_lookup_by_uuid(struct hmap *hmap_p, const struct uuid
*uuid)
HMAP_FOR_EACH_WITH_HASH (answer, uuid_node, uuid_hash(uuid),
hmap_p) {
if (uuid_equals(uuid, answer->uuid)) {
- return(answer);
+ return answer;
}
}
return NULL;
@@ -142,7 +142,7 @@ port_lookup_by_port(const struct ovsrec_port *port)
HMAP_FOR_EACH_WITH_HASH (answer, node, port_hash_rec(port),
&tc.tunnel_hmap) {
if (port == answer->port) {
- return(answer);
+ return answer;
}
}
return NULL;
@@ -288,10 +288,11 @@ check_and_add_tunnel(const struct sbrec_chassis
*chassis_rec,
}
static void
-check_and_update_tunnel(const struct sbrec_chassis *chassis_rec) {
+check_and_update_tunnel(const struct sbrec_chassis *chassis_rec)
+{
struct port_hash_node *port_node;
- if ((port_node =port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
- &chassis_rec->header_.uuid))) {
+ if ((port_node = port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
+ &chassis_rec->header_.uuid))) {
const struct sbrec_encap *encap = preferred_encap(chassis_rec);
const struct ovsrec_port *port = port_node->port;
const struct ovsrec_interface *iface = port->interfaces[0];
@@ -390,7 +391,7 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
&old_hash_node->uuid_node);
}
}
- HMAP_FOR_EACH_POP(old_hash_node, node, &keep_tunnel_hmap_by_uuid) {
+ HMAP_FOR_EACH_POP (old_hash_node, node, &keep_tunnel_hmap_by_uuid) {
;
}
hmap_destroy(&keep_tunnel_hmap_by_uuid);
@@ -416,14 +417,10 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
&port_hash->uuid_node);
free(port_hash);
}
- continue;
- }
- if (!is_new) {
- check_and_update_tunnel(chassis_rec);
- continue;
- } else {
+ } else if (is_new) {
check_and_add_tunnel(chassis_rec, chassis_id);
- continue;
+ } else {
+ check_and_update_tunnel(chassis_rec);
}
}
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
