On Thu, Sep 8, 2016 at 9:39 AM, Justin Pettit <jpet...@ovn.org> wrote: > >> On Sep 2, 2016, at 10:09 PM, Zongkai LI <zealo...@gmail.com> wrote: >> >> This patch updates method is_nd to let type ND_ROUTER_SOLICIT, >> ND_ROUTER_ADVERT, ND_REDIRECT can be recoginzed. >> And introduces method is_nd_neighbor for inherit current is_nd behavior. >> >> v1 -> v2 >> rebased, introduces method is_nd_neighbor. > > I won't mention it for the rest of this series, but please add > "signed-off-by:" and move version information to comment.
Sure, I know it now. > >> --- >> lib/flow.h | 11 +++++++++-- >> lib/nx-match.c | 2 +- >> lib/odp-util.c | 4 ++-- >> ovn/controller/pinctrl.c | 10 ++++++---- >> 4 files changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/lib/flow.h b/lib/flow.h >> index 5b83695..64200a5 100644 >> --- a/lib/flow.h >> +++ b/lib/flow.h >> @@ -936,12 +936,19 @@ static inline bool is_nd(const struct flow *flow, >> if (wc) { >> memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); >> } >> - return (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) || >> - flow->tp_src == htons(ND_NEIGHBOR_ADVERT)); >> + return (flow->tp_src >= htons(ND_ROUTER_SOLICIT) >> + && flow->tp_src <= htons(ND_REDIRECT)); > > You seem to be doing math on network byte order. It probably doesn't affect > the outcome since they're 8-bit values in a 16-bit field, but it doesn't look > right. > > Once again, I don't think we can do anything meaningful with redirect, so it > should probably be dropped. > Sure, I will revert this, and for redirect, since this patch is not for it, I will drop it. >> } >> return false; >> } >> >> +static inline bool is_nd_neighbor(const struct flow *flow, >> + struct flow_wildcards *wc) >> +{ >> + return is_nd(flow, wc) && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) >> + || flow->tp_src == >> htons(ND_NEIGHBOR_ADVERT)); >> +} > > The "is_nd" and "is_nd_neighbor" commands have similar names. I think it > would be worth adding comments to both to help people distinguish the > difference. > Good idea. >> + >> static inline bool is_igmp(const struct flow *flow, struct flow_wildcards >> *wc) >> { >> if (flow->dl_type == htons(ETH_TYPE_IP)) { >> diff --git a/lib/nx-match.c b/lib/nx-match.c >> index b03ccf2..806da3e 100644 >> --- a/lib/nx-match.c >> +++ b/lib/nx-match.c >> @@ -879,7 +879,7 @@ nxm_put_ip(struct ofpbuf *b, const struct match *match, >> enum ofp_version oxm) >> nxm_put_8(b, MFF_ICMPV6_CODE, oxm, >> ntohs(flow->tp_dst)); >> } >> - if (is_nd(flow, NULL)) { >> + if (is_nd_neighbor(flow, NULL)) { >> nxm_put_ipv6(b, MFF_ND_TARGET, oxm, >> &flow->nd_target, &match->wc.masks.nd_target); >> if (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT)) { >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index 6d29b67..0d7bede 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -4428,7 +4428,7 @@ odp_flow_key_from_flow__(const struct >> odp_flow_key_parms *parms, >> icmpv6_key->icmpv6_type = ntohs(data->tp_src); >> icmpv6_key->icmpv6_code = ntohs(data->tp_dst); >> >> - if (is_nd(flow, NULL) >> + if (is_nd_neighbor(flow, NULL) >> /* Even though 'tp_src' and 'tp_dst' are 16 bits wide, ICMP >> * type and code are 8 bits wide. Therefore, an exact match >> * looks like htons(0xff), not htons(0xffff). See >> @@ -4963,7 +4963,7 @@ parse_l2_5_onward(const struct nlattr >> *attrs[OVS_KEY_ATTR_MAX + 1], >> flow->tp_src = htons(icmpv6_key->icmpv6_type); >> flow->tp_dst = htons(icmpv6_key->icmpv6_code); >> expected_bit = OVS_KEY_ATTR_ICMPV6; >> - if (is_nd(src_flow, NULL)) { >> + if (is_nd_neighbor(src_flow, NULL)) { >> if (!is_mask) { >> expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; >> } > > The previous set of changes introduced support for generating and matching > router advertisements, but I don't think you've done any of the work to make > OVS parse the messages. Don't we need that? These changes just seem to > side-step that by only continuing to parse NS and NA messages. > > --Justin > > To be honest, I didn't bring this changing at the beginning. I wanted to put handling for RS in pinctrl_handle_nd_na (and rename pinctrl_handle_nd_na to pinctrl_handle_nd) , while pinctrl_handle_nd_na uses is_nd, so I changed is_nd. For other places calling is_nd, I'm not sure whether should I update them for RS/RA, so I added is_nd_neighbor to inherit original is_nd in behavior. After I completed my patches, I found things work as I expected, VIF can get RA packet after sending RS packet, so I didn't investigate whether should I update processing around is_nd in OVS. I can go investigate that, after I updating native DHCPv6 support in networking-ovn in OpenStack community. But I'm confused why VIF can get RA packet even OVS doesn't get updated for RS/RA. Thanks for your review, Justin. Zongkai, LI _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev