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

Ben Pfaff blp at nicira.com
Wed Feb 2 06:54:06 UTC 2011


On Tue, Feb 1, 2011 at 12:53 AM, Justin Pettit <jpettit at nicira.com> wrote:
> IPv6 uses Neighbor Discovery messages in a similar manner to how IPv4
> uses ARP.  This commit adds support for matching deeper into the
> payloads of Neighbor Solicitation (NS) and Neighbor Advertisement (NA)
> messages.  Currently, the matching fields include:
>
>    - NS and NA Target (nd_target)
>    - NS Source Link Layer Address (nd_sll)
>    - NA Target Link Layer Address (nd_tll)
>
> When defining IPv6 Neighbor Discovery rules, the Nicira Extensible Match
> (NXM) extension to OVS must be used.
>
> Signed-off-by: Justin Pettit <jpettit at nicira.com>

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 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>.

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);

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.

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

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.

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.

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.

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

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




More information about the dev mailing list