[ovs-dev] [IPv6 IV: A New Hope 5/6] nicira-ext: Support matching IPv6 traffic.
jpettit at nicira.com
Wed Feb 2 19:12:36 UTC 2011
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.
> 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.
More information about the dev