[ovs-dev] [PATCH] odp-util: Handle ipv6 in set nw action.

Pravin Shelar pshelar at nicira.com
Mon Jan 9 19:00:46 UTC 2012


On Fri, Jan 6, 2012 at 4:44 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Jan 06, 2012 at 04:01:52PM -0800, Pravin B Shelar wrote:
>> Rather than silently skipping ipv6 action generation, following patch
>> generates OVS_ACTION_ATTR_SET action for ipv6. Datapath which do not
>> support ipv6 action can reject this action.
>
> What is the reason for the changes to ovs_to_odp_frag()?  I guess the
> newer version handles the common case first, which is good, but is
> there another reason?  The logic is equivalent for valid nw_frag
> values, right?  Either way, the change in parameter name is an
> improvement, thanks.
>
yes, checking common case first is the reason for this.

> ipv6_addr_is_zero() will misidentify IPV6 addresses whose 32-bit
> components sum to 0 as being all-zero.  It will cause a bus error on
> RISC architectures if its argument is not 32-bit aligned, which could
> happen if in6_addr has only a byte array member.  I'd do something
> like this:
>
>    static inline int ipv6_addr_is_zero(const struct in6_addr *addr)
>    {
>    #ifdef s6_addr32
>        return !(addr->s6_addr32[0] || addr->s6_addr32[1] ||
>                 addr->s6_addr32[2] || addr->s6_addr32[3]);
>    #else
>        return is_all_zeros(addr, 16);
>    #endif
>    }
>
> You could use | instead of || if you feel inclined.
>
ok.

> But I'm trying to remember why we check that the source and dest are
> nonzero anyway.  I think it must be to make sure that we really have
> an IP header.  I think we could do that less expensively by checking
> that nw_proto is nonzero, since a protocol of 0 isn't useful anyway.
>
ok.

> Do we need any documentation updates?  I think we should at least
> mention in ovs-ofctl.8 that mod_nw_tos works with IPv6, and perhaps
> also add a note to NEWS.
>
> Thanks,
>
> Ben.



More information about the dev mailing list