> On Sep 2, 2016, at 10:09 PM, Zongkai LI <[email protected]> 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.
> ---
> 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.
> }
> 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.
> +
> 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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
