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
