On Feb 20, 2013, at 12:44 PM, Jesse Gross <[email protected]> wrote: > Here are a couple of small comments that I'd already written. I > haven't gone through the main part of the patch yet but I figured that > I might as well send them if you are going to respin the patch. > > On Tue, Feb 19, 2013 at 5:35 PM, Pravin B Shelar <[email protected]> wrote: >> diff --git a/datapath/actions.c b/datapath/actions.c >> index f638ffc..cf44de3 100644 >> --- a/datapath/actions.c >> +++ b/datapath/actions.c >> @@ -439,22 +439,6 @@ static int execute_set_action(struct sk_buff *skb, >> skb_set_mark(skb, nla_get_u32(nested_attr)); >> break; >> >> - case OVS_KEY_ATTR_TUN_ID: >> - /* If we're only using the TUN_ID action, store the value in >> a >> - * temporary instance of struct ovs_key_ipv4_tunnel on the >> stack. >> - * If both IPV4_TUNNEL and TUN_ID are being used together we >> - * can't write into the IPV4_TUNNEL action, so make a copy >> and >> - * write into that version. >> - */ >> - if (!OVS_CB(skb)->tun_key) >> - memset(tun_key, 0, sizeof(*tun_key)); >> - else if (OVS_CB(skb)->tun_key != tun_key) >> - memcpy(tun_key, OVS_CB(skb)->tun_key, >> sizeof(*tun_key)); >> - OVS_CB(skb)->tun_key = tun_key; >> - >> - OVS_CB(skb)->tun_key->tun_id = nla_get_be64(nested_attr); >> - break; > > I think we can further simplify this by removing the temporary tun_key > that we are keeping on the stack in ovs_execute_actions() and passing > around. > >> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h >> index 7ee31a2..12e85b2 100644 >> --- a/include/linux/openvswitch.h >> +++ b/include/linux/openvswitch.h >> @@ -242,6 +242,18 @@ enum { >> >> #define OVS_PATCH_ATTR_MAX (__OVS_PATCH_ATTR_MAX - 1) >> >> +/* OVS_VPORT_ATTR_OPTIONS attributes for tunnels. >> + * >> + * OVS_TUNNEL_ATTR_DST_PORT is useful for vxlan. > > I'm not sure that we need this comment about VXLAN here since it's > probably easier to just describe each element next to it. > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev
Another thing I just noticed: I don't see where tnl_vport->dst_port is set when creating a VXLAN port. I see it set when setting tunnel options, but not during create. I think this explains why when I tested this patch with VXLAN I see the destination port being zero during transmit. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
