On Tue, Oct 01, 2013 at 04:02:17PM -0700, Jesse Gross wrote: > On Mon, Sep 30, 2013 at 11:47 PM, Simon Horman <ho...@verge.net.au> wrote: > > diff --git a/datapath/actions.c b/datapath/actions.c > > index d961e5d..bfab9ec 100644 > > --- a/datapath/actions.c > > +++ b/datapath/actions.c > > +/* Push MPLS after the ethernet header. */ > > +static int push_mpls(struct sk_buff *skb, > > + const struct ovs_action_push_mpls *mpls) > [...] > > + hdr = eth_hdr(skb); > > + hdr->h_proto = mpls->mpls_ethertype; > > + if (!eth_p_mpls(skb->protocol) && !ovs_skb_get_inner_protocol(skb)) > > + ovs_skb_set_inner_protocol(skb, skb->protocol); > > Do we actually need the check for !eth_p_mpls(skb->protocol)? It's not > clear to me what condition it is trying to prevent.
True, it does not seem useful. I will remove it. > > +static int pop_mpls(struct sk_buff *skb, const __be16 ethertype) > > +{ > > + struct ethhdr *hdr; > > + int err; > > + > > + err = make_writable(skb, skb->mac_len + MPLS_HLEN); > > + if (unlikely(err)) > > + return err; > > + > > + if (unlikely(skb->len < skb->mac_len + MPLS_HLEN)) > > + return -ENOMEM; > > I'm not sure that this is the right error code in this situation - > maybe -EINVAL would be better. I was a bit unsure what was an appropriate value but I agree that -EINVAL seems better than -ENOMEM. I will change it to -EINVAL. > > > @@ -545,6 +662,14 @@ static int do_execute_actions(struct datapath *dp, > > struct sk_buff *skb, > > output_userspace(dp, skb, a); > > break; > > > > + case OVS_ACTION_ATTR_PUSH_MPLS: > > + err = push_mpls(skb, nla_data(a)); > > + break; > > + > > + case OVS_ACTION_ATTR_POP_MPLS: > > + err = pop_mpls(skb, nla_get_be16(a)); > > + break; > > I think we need something similar to POP_VLAN here - in the event of > an error the skb will have already been freed. Thanks, though I think you mean that PUSH_MPLS should be similar to PUSH_VLAN. I don't think that either pop_mpls() or pop_vlan() free the skb on error. I will add the following: diff --git a/datapath/actions.c b/datapath/actions.c index bfab9ec..8babfc4 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -664,6 +664,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, case OVS_ACTION_ATTR_PUSH_MPLS: err = push_mpls(skb, nla_data(a)); + if (unlikely(err)) /* skb already freed. */ + return err; break; case OVS_ACTION_ATTR_POP_MPLS: > > > diff --git a/datapath/datapath.h b/datapath/datapath.h > > index 4a49a7d..31fe10a 100644 > > --- a/datapath/datapath.h > > +++ b/datapath/datapath.h > > @@ -95,6 +95,8 @@ struct datapath { > > * @pkt_key: The flow information extracted from the packet. Must be > > nonnull. > > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the > > * packet is not being tunneled. > > + * @inner_protocol: Provides a substitute for the skb->inner_protocol > > field on > > + * kernels before 3.11. > > */ > > struct ovs_skb_cb { > > struct sw_flow *flow; > > I think this comment no longer applies now that inner_protocol has > been moved into the GSO struct. Thanks, I'll clean that up. > > diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c > > index 32f906c..f917356 100644 > > --- a/datapath/linux/compat/gso.c > > +++ b/datapath/linux/compat/gso.c > > int rpl_dev_queue_xmit(struct sk_buff *skb) > > { > > #undef dev_queue_xmit > > int err = -ENOMEM; > > + __be16 inner_protocol; > > + bool vlan, mpls; > > > > - if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev)) { > > + vlan = mpls = false; > > + > > + inner_protocol = ovs_skb_get_inner_protocol(skb); > > I don't think this is actually used in this function. Thanks, I guess that is the by-product of some refactoring. I'll remove it. > Pravin, do you have any further comments? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev