> On Aug 27, 2015, at 11:21 PM, Alex Wang <[email protected]> wrote:
>
>
> +/* Searches the 'chassis_rec->encaps' for the first vtep tunnel
> + * configuration, returns the 'ip'. */
> +static const char *
> +get_chassis_vtep_ip(const struct sbrec_chassis *chassis_rec)
It might be nice to indicate to the caller the length of time the returned
value can be used, since they don't own the string.
> +/* Creates a new 'Physical_Locator'. */
> +static struct vteprec_physical_locator *
> +create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
> +{
> + struct vteprec_physical_locator *new_pl;
> +
> + new_pl = vteprec_physical_locator_insert(vtep_idl_txn);
> + vteprec_physical_locator_set_dst_ip(new_pl, chassis_ip);
> + vteprec_physical_locator_set_encapsulation_type(new_pl, VTEP_ENCAP_TYPE);
We're looking at adding IPv6 tunnel support into OVS. It might be nice to to
called this definition VTEPv4_ENCAP_TYPE or something similar.
> + SHASH_FOR_EACH (node, vtep_lswitches) {
> + struct ls_hash_node *ls_node = xmalloc(sizeof *ls_node);
> +
> + ls_node->vtep_ls = node->data;
> + shash_init(&ls_node->added_macs);
> + hmap_insert(&ls_map, &ls_node->hmap_node,
> + hash_uint64((uint64_t) ls_node->vtep_ls->tunnel_key[0]));
Can't "ls_node->vtep_ls->tunnel_key" be NULL?
>
> + SHASH_FOR_EACH (node, other_pbs) {
> + const struct sbrec_port_binding *port_binding_rec = node->data;
> + const struct sbrec_chassis *chassis_rec;
> + struct ls_hash_node *ls_node;
> + const char *chassis_ip;
> + int64_t tnl_key;
> + size_t i;
> +
> + chassis_rec = port_binding_rec->chassis;
> + if (!chassis_rec) {
> + continue;
> + }
> +
> + tnl_key = port_binding_rec->datapath->tunnel_key;
> + HMAP_FOR_EACH_WITH_HASH (ls_node, hmap_node,
> + hash_uint64((uint64_t) tnl_key),
> + &ls_map) {
> + if (ls_node->vtep_ls->tunnel_key[0] == tnl_key) {
Once again, can't this result in a NULL comparison?
> + /* If finds the 'umr' entry for the mac and ip, deletes the
> + * entry from shash so that it is not gargage collected.
> + *
> + * If not found, creates a new 'umr' entry.
> + *
> + * If vtep logical switch does not match, the logical port
> + * owning the MAC may be moved to a new ovn logical datapath.
Couldn't they also just have overlapping addresses?
> + /* Collects 'Ucast_Macs_Remote's. */
> + VTEPREC_UCAST_MACS_REMOTE_FOR_EACH (umr, ctx->vtep_idl) {
> + char *mac_ip =
> + xasprintf("%s%s", umr->MAC,
> + umr->locator ? umr->locator->dst_ip : "");
> +
> + shash_add(&ucast_macs_rmts, mac_ip, umr);
> + free(mac_ip);
> + }
Based on how this is used in vtep_macs_run(), don't you need to account for
overlapping MAC addresses? I'd think you'd also need to take into account the
logical switch tunnel key to help disambiguate that case.
> + struct shash vtep_pbs = SHASH_INITIALIZER(&vtep_pbs);
> + struct shash other_pbs = SHASH_INITIALIZER(&other_pbs);
I think "non_vtep_pbs" or something similar may be a helpful name. Later in
the code, it becomes unclear what these are "other" from.
> + /* Collects 'Ucast_Macs_Remote's. */
> + VTEPREC_UCAST_MACS_REMOTE_FOR_EACH (umr, ctx->vtep_idl) {
> + char *mac_ip =
> + xasprintf("%s%s", umr->MAC,
> + umr->locator ? umr->locator->dst_ip : "");
xxx Surprised there's no delimiter.
> + shash_destroy(&vtep_lswitches);
> + shash_destroy(&ucast_macs_rmts);
> + shash_destroy(&physical_locators);
> + shash_destroy(&vtep_pbs);
> + shash_destroy(&other_pbs);
I think you're missing a sset_destroy(&vtep_pswitches).
Acked-by: Justin Pettit <[email protected]>
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
