[ovs-dev] [PATCH v2 4/8] flow: update is_nd for all ND msg types

[Zong Kai Li]

On Thu, Sep 8, 2016 at 9:39 AM, Justin Pettit <jpettit at ovn.org> wrote:
>
>> On Sep 2, 2016, at 10:09 PM, Zongkai LI <zealokii at 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