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.
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.
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.
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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
