[ovs-dev] [PATCH v5 2/3] ovn-controller: Add a new action - 'put_nd_ra_opts'

Ben Pfaff blp at ovn.org
Thu Aug 3 21:32:24 UTC 2017


On Mon, Jul 31, 2017 at 06:11:35PM +0530, nusiddiq at redhat.com wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> This patch adds a new OVN action 'put_nd_ra_opts' to support native
> IPv6 Router Advertisement in OVN. This action can be used to respond
> to the IPv6 Router Solicitation requests.
> 
> ovn-controller parses this action and adds a NXT_PACKET_IN2 OF flow
> with 'pause' flag set and the RA options stored in 'userdata' field.
> This action is similar to 'put_dhcp_opts' and 'put_dhcpv6_opts'.
> 
> When a valid IPv6 RS packet is received by the pinctrl module of
> ovn-controller, it frames a new RA packet and sets the RA options
> from the 'userdata' field and resumes the packet storing 1 in the
> 1-bit result sub-field. If the packet is invalid, it resumes the
> packet without any modifications storing 0 in the 1-bit result
> sub-field.
> 
> Eg. reg0[5] = put_nd_ra_opts(address_mode = "slaac", mtu = 1450,
>                              slla = 01:02:03:04:05:06, prefix = aef0::/64)
> 
> Note that unlike DHCPv4/v6, a new table to store the supported IPv6 ND RA
> options is not added in SB DB since there are only 3 ND RA options.
> 
> Co-authored-by: Zongkai LI <zealokii at gmail.com>
> Signed-off-by: Zongkai LI <zealokii at gmail.com>
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> Acked-by: Miguel Angel Ajo <majopela at redhat.com>

Thanks for working on this.

Looking at it, the treatment of some of the options strikes me as odd.
Some of the options (SLL) are actually required, and others could be
supplied as fixed data since there's a default value (mo_flags, mtu).
Only the prefixes seem to really be options in what I think of as the
usual sense.  It looks to me like those prefixes could be supplied
directly in the userdata as bytes to append to the packet.  It might be
cleaner to make these changes--then there would be less parsing of text
options, encoding them as binary options, decoding those binary options,
and then re-encoding them again in the packet.

If that doesn't make sense, though, this ALIGNED_CAST in
pinctrl_handle_put_nd_ra_opts()looks wrong--I see no reason to believe
that opt_data is 32-bit aligned.  Probably should use
get_unaligned_be32():
            if (opt_len == sizeof(ovs_be32)) {
                mtu = *(ALIGNED_CAST(const ovs_be32 *, opt_data));
            }

Thanks,

Ben.


More information about the dev mailing list