On 13 September 2015 at 10:45, Justin Pettit <jpet...@nicira.com> wrote:
>
> > On Aug 27, 2015, at 11:21 PM, Alex Wang <ee07b...@gmail.com> 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.
>
>
added the following changes:
/* Searches the 'chassis_rec->encaps' for the first vtep tunnel
- * configuration, returns the 'ip'. */
+ * configuration, returns the 'ip'. Unless duplicated, the returned
+ * pointer cannot live past current vtep_run() execution. */
> > +/* 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.
>
>
I'd like to do this in another patch~
> > + 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?
>
Sorry for the unclarity, at the bottom of vtep_lswitch_run(), I reset
the tunnel key for all unused logical switches. so, the vtep_ls->tunnel_key
will not be NULL after vtep_lswitch_run().
How about adding a comment like this:
+ /* 'vtep_ls->tunnel_key' will not be NULL, since vtep_lswitch_run()
+ * sets/resets the 'tunnel_key' for all logical switches. */
hmap_insert(&ls_map, &ls_node->hmap_node,
hash_uint64((uint64_t)
ls_node->vtep_ls->tunnel_key[0]));
}
or I can add a NULL pointer check,
>
> >
> > + 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.
>
>
Thx for pointing this out, I'll use mac_ip_tnlkey to distinguish
> > + 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.
>
>
Sure, changed,
> > + /* 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.
>
is that a convention? since I only need it for hash, just put them all
together,
>
> > + 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).
>
Thx, fixed,
Will send out a new series~~
> Acked-by: Justin Pettit <jpet...@nicira.com>
>
> --Justin
>
>
>
--
Alex Wang,
Open vSwitch developer
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
