[ovs-dev] [ovn-controller-vtep V6 6/7] ovn-controller-vtep: Extend vtep module to install Ucast_Macs_Remote.

[Russell Bryant]

On 08/17/2015 09:06 AM, Alex Wang wrote:
> 
> 
> On Mon, Aug 17, 2015 at 6:37 AM, Russell Bryant <rbryant at redhat.com
> <mailto:rbryant at redhat.com>> wrote:
> 
>     On 08/16/2015 07:24 PM, Alex Wang wrote:
>     >
>     >
>     > On Sun, Aug 16, 2015 at 3:26 PM, Russell Bryant <rbryant at redhat.com <mailto:rbryant at redhat.com>
>     > <mailto:rbryant at redhat.com <mailto:rbryant at 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