Wow, this is such an awesome change!!! I totally like it~ Could you send the patch? Would like to know what Ben thinks about this,
Thanks, Alex Wang, On Mon, Aug 17, 2015 at 9:43 AM, Russell Bryant <rbry...@redhat.com> wrote: > On 08/17/2015 09:06 AM, Alex Wang wrote: > > > > > > On Mon, Aug 17, 2015 at 6:37 AM, Russell Bryant <rbry...@redhat.com > > <mailto:rbry...@redhat.com>> wrote: > > > > On 08/16/2015 07:24 PM, Alex Wang wrote: > > > > > > > > > On Sun, Aug 16, 2015 at 3:26 PM, Russell Bryant < > rbry...@redhat.com <mailto:rbry...@redhat.com> > > > <mailto:rbry...@redhat.com <mailto:rbry...@redhat.com>>> wrote: > > > > > > On 08/09/2015 10:50 PM, Alex Wang wrote: > > > > + 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) > { > > > > + break; > > > > + } > > > > + } > > > > + /* If 'ls_node' is NULL, that means no vtep logical > switch is > > > > + * attached to the corresponding ovn logical > datapath, so pass. */ > > > > + if (!ls_node) { > > > > + continue; > > > > + } > > > > > > I don't think ls_node is guaranteed to be NULL here, even when > the loop > > > ends normally (without a break). > > > > > > > > > Could you explain more about this concern? From the comments in > > > lib/hmap.h, this looks okay to me. > > > > Ah, you're right. It's fine since hmap_node is the first member in > the > > struct. Thanks. > > > > It might be worth a comment in your struct definition saying that > > hmap_node must remain the first struct member. > > > > > > Yeah, I really should do that! Thx, > > I wonder if we can remove the requirement to hvae hmap_node as the first > element. This patch at least compiles and the tests pass. What do you > think? > > > diff --git a/lib/hmap.h b/lib/hmap.h > > index 345bf7f..9cb3ad4 100644 > > --- a/lib/hmap.h > > +++ b/lib/hmap.h > > @@ -136,12 +136,12 @@ struct hmap_node *hmap_random_node(const struct > hmap *); > > */ > > #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) > \ > > for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), > MEMBER); \ > > - NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); > \ > > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = > NULL); \ > > ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER), > \ > > MEMBER)) > > #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) > \ > > for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), > MEMBER); \ > > - NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); > \ > > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = > NULL); \ > > ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), > MEMBER)) > > > > static inline struct hmap_node *hmap_first_with_hash(const struct hmap > *, > > @@ -158,14 +158,14 @@ bool hmap_contains(const struct hmap *, const > struct hmap_node *); > > /* Iterates through every node in HMAP. */ > > #define HMAP_FOR_EACH(NODE, MEMBER, HMAP) > \ > > for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER); > \ > > - NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); > \ > > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = > NULL); \ > > ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), > MEMBER)) > > > > /* Safe when NODE may be freed (not needed when NODE may be removed > from the > > * hash map but its members remain accessible and intact). */ > > #define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP) > \ > > for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER); > \ > > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) > \ > > + ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = > NULL) \ > > ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), > MEMBER), 1 \ > > : 0); > \ > > (NODE) = (NEXT)) > > @@ -173,7 +173,7 @@ bool hmap_contains(const struct hmap *, const struct > hmap_node *); > > /* Continues an iteration from just after NODE. */ > > #define HMAP_FOR_EACH_CONTINUE(NODE, MEMBER, HMAP) > \ > > for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), > MEMBER); \ > > - NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); > \ > > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = > NULL); \ > > ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), > MEMBER)) > > > > static inline struct hmap_node *hmap_first(const struct hmap *); > > > -- > Russell Bryant > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
