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

Alex Wang alexw at nicira.com
Tue Aug 18 01:10:10 UTC 2015


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 <rbryant at redhat.com> wrote:

> 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
>



More information about the dev mailing list