[ovs-dev] [PATCH] WIP OVN ND for Logical_Port

[Zong Kai LI]

On Tue, Jun 7, 2016 at 5:08 AM, Ben Pfaff <blp at ovn.org> wrote:

> On Fri, May 27, 2016 at 10:32:56AM +0800, Zong Kai LI wrote:
> > From: lzklibj <lzklibj at cn.ibm.com>
> >
> > This patch tries to implement ND for Logical_Port in OVN.
> >
> > Signed-off-by: lzklibj <lzklibj at cn.ibm.com>
>
> Thanks for working on this.
>
> I'm CCing Justin, who is working on related features, to bring this
> patch to his attention.
>
> "sparse" says:
>     ../ovn/controller/pinctrl.c:748:25: warning: cast to restricted
> ovs_be16
> about this line:
>     na->rco_flags.hi = (ovs_be16)0x60;
> Probably, you should use put_16aligned_be32().
>
> In pinctrl.c, please #include <netinet/icmp6.h> after pinctrl.h.
> Keeping pinctrl.h first ensures that it is self-contained.
>
> The functions compose_na() and reload_metadata() in pinctrl.c should be
> declared static, unless you want to make them public (and in that case
> the prototypes should be in a header file instead).
>
> I am concerned about alignment issues.  I am not sure that the use of
> ALIGNED_CAST here is safe on RISC platforms, because IPv6 addresses
> aren't always aligned on a 32-bit boundary when they are inside Ethernet
> packets with different encapsulations.  It would probably be better to
> use a different strategy to assure alignment, rather than to just assume
> it.
>
> Please add documentation for the new action, in ovn-sb.xml.
>
> I see that there are a few notes and to-do items; I guess that's what
> makes it a work-in-progress.
>
> Thanks,
>
> Ben.
>

Hi, Ben. Thanks for your time and review.

I know Justin is working on implement router patch port for IPv6 switch,
but I'm not sure whether will Justin also work on implement ND or not.
And I've separated the WIP patch to:
 - 1) ovn-controller: Add 'na' action for ND
 - 2) ovn: add lflows for 'na' action for ND
it goes to version 3 now, and I just updated test in 1). Not sure what kind
of test should be added in 2) yet.

About the comments you gave:
 - the function compose_na(), I've moved it to lib/packets.h and
lib/packets.c, and it's public function now. And after that,
<netinet/icmp6.h> is no longer needed in pinctrl.c.
 - the function reload_metadata, I keep it in pinctrl.c and make it static.
 - about na->rco_flags.hi, the na.rco_flags is in a struct has two ovs_be16
type variable, hi and lo. So I think may be it's OK to directly assign the
hi variable a ovs_be16 type value.
 - about ALIGNED_CAST, indeed, I copied most part of compose_nd() to
implement compose_na(), so I suppose they could work fine.

Thanks again & have a nice day! :)

Best regards,
Zong Kai, LI