[ovs-dev] [IPv6 IV: A New Hope 5/6] nicira-ext: Support matching IPv6 traffic.

[Justin Pettit]

On Feb 2, 2011, at 10:36 AM, Ben Pfaff wrote:

> I believe that in parse_ipv6hdr(), the statement
> 	nh_len = payload_ofs - skb_network_offset(skb);
> can be simplified to:
> 	nh_len = payload_ofs - nh_ofs;
> because I don't see anything in between nh_ofs and nh_len that would
> change the network header offset.

Before using ipv6_skip_exthdr(), I did a bunch of SKB pulls that could change that offset as I processed extension headers.  That function doesn't do pulls, so it's not needed any more.  Thanks.

> Why does parse_ipv6hdr() return the network header length?  I don't see
> where the caller uses it.  I think that it could just return 0.

It's used in the following patch to figure out how long the ICMP payload is.  Rather than change the calling convention between patches, I just do it the way I'm going to need it here.

> In odp_key_ipv6, the uint32_t fields should be ovs_be32.

Whoop.  Thanks.

> In odp_flow_key_to_flow(), I don't really like the mixing of IP_TYPE_*
> (from packets.h) with IPPROTO_* (from some system header).  I'd rather
> add a IP_TYPE_ICMPV6 or use IPPROTO_* uniformly.  Ditto in
> parse_protocol() in ofp-parse.c.

I agree and was planning to clean that up in a follow-on patch.  Do you know why we define IP_TYPE_* in packets.h, if they're defined in "netinet/in.h"?  I'd prefer to use the pre-defined ones and get rid of "IP_TYPE_*" from packets.h unless there's a compelling reason not to.

> Acked-by: Ben Pfaff <blp at nicira.com>


Thank you very much for the reviews, Ben and Jesse!  Ben, I'll wait to hear back from you on the above point before pushing the changes.

--Justin