Remove the global set of logical port IDs called 'all_lports'. This is no longer used for anything after conntrack ID assignment was moved out of binding.c.
Remove the global smap of logical port IDs to ovsrec_interface records. We can't persist references to these records, as we may be holding references to freed memory. Instead, replace it with a new global sset of logical port IDs called 'local_ids'. This is used to track when interfaces have been added or removed. Found by inspection. Signed-off-by: Russell Bryant <russ...@ovn.org> --- ovn/controller/binding.c | 101 ++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 45 deletions(-) diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 4704226..8e6d17a 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -27,8 +27,10 @@ VLOG_DEFINE_THIS_MODULE(binding); -static struct sset all_lports = SSET_INITIALIZER(&all_lports); +/* A set of the iface-id values of local interfaces on this chassis. */ +static struct sset local_ids = SSET_INITIALIZER(&local_ids); +/* When this gets set to true, the next run will re-check all binding records. */ static bool process_full_binding = false; void @@ -60,14 +62,13 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) } static bool -get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports) +get_local_iface_ids(const struct ovsrec_bridge *br_int) { int i; bool changed = false; - /* A local copy of ports that we can use to compare with the persisted - * list. */ - struct shash local_ports = SHASH_INITIALIZER(&local_ports); + struct sset old_local_ids = SSET_INITIALIZER(&old_local_ids); + sset_clone(&old_local_ids, &local_ids); for (i = 0; i < br_int->n_ports; i++) { const struct ovsrec_port *port_rec = br_int->ports[i]; @@ -86,25 +87,21 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports) if (!iface_id) { continue; } - shash_add(&local_ports, iface_id, iface_rec); - if (!shash_find(lports, iface_id)) { - shash_add(lports, iface_id, iface_rec); + if (!sset_find_and_delete(&old_local_ids, iface_id)) { + sset_add(&local_ids, iface_id); changed = true; } - if (!sset_find(&all_lports, iface_id)) { - sset_add(&all_lports, iface_id); - binding_reset_processing(); - } } } - struct shash_node *iter, *next; - SHASH_FOR_EACH_SAFE(iter, next, lports) { - if (!shash_find_and_delete(&local_ports, iter->name)) { - shash_delete(lports, iter); - changed = true; - } + + /* Any item left in old_local_ids is an ID for an interface + * that has been removed. */ + if (!changed && !sset_is_empty(&old_local_ids)) { + changed = true; } - shash_destroy(&local_ports); + + sset_destroy(&old_local_ids); + return changed; } @@ -129,7 +126,6 @@ static void remove_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld) { if (ld->logical_port) { - sset_find_and_delete(&all_lports, ld->logical_port); free(ld->logical_port); ld->logical_port = NULL; } @@ -187,21 +183,48 @@ update_qos(const struct ovsrec_interface *iface_rec, ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst)); } +/* Return an ovsrec_interface that has an iface-id matching lport. */ +static const struct ovsrec_interface * +iface_for_lport(const struct ovsrec_bridge *br_int, const char *lport) +{ + int i; + + for (i = 0; i < br_int->n_ports; i++) { + const struct ovsrec_port *port_rec = br_int->ports[i]; + const char *iface_id; + int j; + + if (!strcmp(port_rec->name, br_int->name)) { + continue; + } + + for (j = 0; j < port_rec->n_interfaces; j++) { + const struct ovsrec_interface *iface_rec; + + iface_rec = port_rec->interfaces[j]; + iface_id = smap_get(&iface_rec->external_ids, "iface-id"); + if (iface_id && !strcmp(iface_id, lport)) { + return iface_rec; + } + } + } + + return NULL; +} + static void -consider_local_datapath(struct controller_ctx *ctx, struct shash *lports, +consider_local_datapath(struct controller_ctx *ctx, const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding *binding_rec, - struct hmap *local_datapaths) + struct hmap *local_datapaths, + const struct ovsrec_bridge *br_int) { const struct ovsrec_interface *iface_rec - = shash_find_data(lports, binding_rec->logical_port); + = iface_for_lport(br_int, binding_rec->logical_port); + if (iface_rec || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(&all_lports, binding_rec->parent_port))) { - if (binding_rec->parent_port && binding_rec->parent_port[0]) { - /* Add child logical port to the set of all local ports. */ - sset_add(&all_lports, binding_rec->logical_port); - } + sset_contains(&local_ids, binding_rec->parent_port))) { add_local_datapath(local_datapaths, binding_rec, &binding_rec->header_.uuid); if (iface_rec && ctx->ovs_idl_txn) { @@ -242,7 +265,6 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports, VLOG_INFO("Claiming l2gateway port %s for this chassis.", binding_rec->logical_port); sbrec_port_binding_set_chassis(binding_rec, chassis_rec); - sset_add(&all_lports, binding_rec->logical_port); add_local_datapath(local_datapaths, binding_rec, &binding_rec->header_.uuid); } @@ -253,20 +275,9 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports, binding_rec->logical_port); sbrec_port_binding_set_chassis(binding_rec, NULL); } - } else if (!binding_rec->chassis - && !strcmp(binding_rec->type, "localnet")) { - /* Localnet ports will never be bound to a chassis, but we want - * to list them in all_lports because we want to allocate - * a conntrack zone ID for each one, as we'll be creating - * a patch port for each one. */ - sset_add(&all_lports, binding_rec->logical_port); } } -/* We persist lports because we need to know when it changes to - * handle ports going away correctly in the binding record. */ -static struct shash lports = SHASH_INITIALIZER(&lports); - void binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const char *chassis_id, struct hmap *local_datapaths) @@ -280,7 +291,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, } if (br_int) { - if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lports)) { + if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int)) { process_full_binding = true; } } else { @@ -296,8 +307,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, struct hmap keep_local_datapath_by_uuid = HMAP_INITIALIZER(&keep_local_datapath_by_uuid); SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { - consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, - local_datapaths); + consider_local_datapath(ctx, chassis_rec, binding_rec, + local_datapaths, br_int); struct local_datapath *ld = xzalloc(sizeof *ld); memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid); hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node, @@ -317,8 +328,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, if (sbrec_port_binding_is_deleted(binding_rec)) { remove_local_datapath_by_binding(local_datapaths, binding_rec); } else { - consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, - local_datapaths); + consider_local_datapath(ctx, chassis_rec, binding_rec, + local_datapaths, br_int); } } } -- 2.7.4 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev