On Wed, Jul 3, 2013 at 9:11 AM, Andy Zhou <az...@nicira.com> wrote: > Reported-by: Justin Pettit <jpet...@nicira.com> > Signed-off-by: Andy Zhou <az...@nicira.com> > --- > datapath/flow.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/datapath/flow.c b/datapath/flow.c > index 2ac36b6..c598641 100644 > --- a/datapath/flow.c > +++ b/datapath/flow.c > @@ -1251,15 +1251,14 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb, > return -EMSGSIZE; > if (nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST, output->ipv4_dst)) > return -EMSGSIZE; > - if (tun_key->ipv4_tos && > - nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos)) > + if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos)) > return -EMSGSIZE; > if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl)) > return -EMSGSIZE; > - if ((tun_key->tun_flags & TUNNEL_DONT_FRAGMENT) && > + if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) && > nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT)) > return -EMSGSIZE; > - if ((tun_key->tun_flags & TUNNEL_CSUM) && > + if ((output->tun_flags & TUNNEL_CSUM) && > nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_CSUM)) > return -EMSGSIZE;
I think this patch is correct but needs to go farther. For the masks, at least, we need to always output the attribute. For example, if the TUNNEL_KEY flag isn't set in the key but it is in the mask that means that there must not be a tunnel key. However, with the current logic we wouldn't output the mask, which means that we don't care. It's safe to output a key attribute that is zero, even if it isn't strictly necessary, so we can just error on the side of doing that to simplify the code. Really the only place where we should look at the key is when figuring out the prerequisites for later protocols. Can you look through the other serialization code? I think we have the same problem other places as well. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
