On Fri, Jan 6, 2012 at 4:44 PM, Ben Pfaff <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
