[ovs-dev] [IPv6 IV: A New Hope 6/6] nicira-ext: Support matching IPv6 Neighbor Discovery messages.

Justin Pettit jpettit at nicira.com
Wed Feb 2 07:59:32 UTC 2011


On Feb 1, 2011, at 10:54 PM, Ben Pfaff wrote:

> I looked over the datapath/flow.c and lib/flow.c changes pretty
> carefully, although it is a bit late for me tonight.  I don't see any
> actual problems.
> 
> Some of the kernel code looks wrongly indented, but that might be
> because I'm viewing this in the online gmail window.

I tried to be careful not to introduce any tab/space issues, and another quick look didn't show any.  I don't know of any other types of inconsistencies, but I'll happily fix any that you point out.

> I don't think datapath/flow.c needs a new function
> eth_addr_is_zero().  There's already is_zero_ether_addr() in
> <linux/etherdevice.h>.

Ah, I did a quick search initially and missed that.  Thanks!

> There's a couple of datapath/flow.c code blocks of the form
> 	   if (eth_addr_is_zero(key->arp_sha)) {
> 		   memcpy(key->arp_sha,
> 				   &nd->opt[offset+sizeof(*nd_opt)], ETH_ALEN);
> 	   } else {
> 		   goto invalid;
> 	   }
> that would be more conventionally (for the kernel) written:
> 	if (!is_zero_ether_addr(key->arp))
> 		goto invalid;
> 	memcpy(key->arp_sha, &nd->opt[offset+sizeof(*nd_opt)], ETH_ALEN);

Thanks for the style pointers.  I went with the preferred kernel convention.

> The parse_icmpv6() return value makes it unnecessarily hard for the
> caller: if it just returned 0 for invalid icmpv6 then the caller
> wouldn't have to do two comparisons.

Yeah, I guess we handle all the cleanup of the key in that function and no further processing is done, so it's fine to not return EINVAL at all.  Done.

> flow_to_nlattrs() has a couple of sizeofs without parentheses around
> their operands.

Thanks.

> In lib/flow.c, it's most common in our userspace to use positive errno
> values, when possible to represent errors, instead of the negative
> ones used in parse_icmpv6().  Or it could just return a bool, since
> there's only one kind of error.

I went with bool.

> Also in lib/flow.c, the loop is a little more cautious than necessary:
> the (icmp_len >= 8) condition will ensure that ofpbuf_try_pull(b, 8)
> always returns nonnull and thus you might as well use ofpbuf_pull(b,
> 8) and not both with checking for nonnull.

True enough.  Done.

> I don't see a definition of nd_msg or nd_opt_hdr or related structures
> anywhere.  I could easily be missing it, but if there aren't any then
> I'd expect that we should add definitions to lib/packets.h so that we
> can build on non-Linux hosts.  But if we're in a hurry that can wait.

"nd_msg" is only used in the kernel and defined in "include/net/ndisc.h" all the way back to 2.6.18.8.  The same is true of "nd_opt_hdr" in the kernel.  In userspace, "nd_opt_hdr" is defined in "/usr/include/netinet/icmp6.h".

> How portable is <netinet/icmp6.h>?  I don't see it in POSIX...  Again
> that could wait if we're in a hurry.

I think it's fairly portable.  It's described in RFC 2292 with the definitions we need, and I had verified that it exists on my Mac and NetBSD boxes.

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

Thank you for the thorough review!  I'll wait for your acks on 3 and 5 before I push.

--Justin






More information about the dev mailing list