[ovs-dev] [PATCHv2 2/2] datapath: add ipv6 'set' action

[Jesse Gross]

I noticed a couple of minor things in the userspace code but it looks
basically good to me.

On Thu, Nov 8, 2012 at 5:14 AM, Ansis Atteka <aatteka at nicira.com> wrote:

> diff --git a/lib/packets.c b/lib/packets.c
> index 16f4fe6..6cb3a80 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> +static bool
> +packet_rh_present(struct ofpbuf *packet)
> [...]
> +        } else if (nexthdr == IPPROTO_ROUTING) {
> +            const struct ip6_rthdr *rh = (struct ip6_rthdr *)data;
> +
> +            if (remaining < sizeof *rh) {
> +                return false;
> +            }
>

Isn't all the information we need in the routing header in the first 8
bytes (and therefore caught by the general check)?


> +static void
> +packet_set_ipv6_flow_label(ovs_be32 *flow_label, uint32_t flow_key)
> +{
> +    *flow_label = htonl((ntohl(*flow_label) & 0xFFF00000) | flow_key);
>

I would probably use ~IPV6_LABEL_MASK.  It's also usually better to do the
byte swap on the constant since it can be done at compile time.  It doesn't
matter much here but it's free.  Something like this and similarly for
packet_set_ipv6_tc:
*flow_label = (*flow_label & htonl(0xFFF00000)) | htonl(flow_key);

Actually in this case, the caller has already byte swapped flow_key, so we
could just drop both.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121109/a270339f/attachment-0003.html>