[ovs-dev] [PATCH v1 3/3] ovn-northd: Add logical flows to support DHCPv6

[Ben Pfaff]

On Wed, Jul 27, 2016 at 12:55:24AM +0530, Numan Siddique wrote:
> OVN implements native DHCPv6. DHCPv6 options are stored
> in the 'DHCP_Options' NB table and logical ports refer to this
> table to configure the DHCPv6 options.
> 
> For each logical port configured with DHCPv6 Options following flows
> are added
>  - A logical flow which copies the DHCPv6 options to the DHCPv6
>    request packets using the 'put_dhcpv6_opts' action and advances the
>    packet to the next stage.
> 
>  - A logical flow which implements the DHCPv6 reponder by sending
>    the DHCPv6 reply back to the inport once the 'put_dhcpv6_opts' action
>    is applied.
> 
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>

I think that the change to packets.c is a mistake; I don't see a reason
for it.

build_dhcpv6_action() could use ipv6_mask_is_any() instead of checking
for zeros by hand.  Actually ipv6_mask_is_any() is a pretty terrible
name for that function, it at least needs a comment to indicate that it
checks for all-zeros.

(Also not all systems define the s6_addr32 member of struct in6_addr.)

It seems fairly obnoxious to give the DHCPv6 options all-caps names,
could we make them lowercase instead?

A buffer for formatting an IPv6 address should be INET6_ADDRSTRLEN bytes
long, not IPV6_SCAN_LEN.  I guess they're the same number but
conceptually fairly different.

I don't see why build_dhcpv6_action() and build_acls() memset the string
buffers before formatting into them.

Like DHCPv4, I don't think that nothing_to_add is a safe optimization in
practice.  It'll burn us eventually.

Please write
+        }
+        else {
as
+        } else {

Thanks,

Ben.