On Wed, May 22, 2013 at 6:38 PM, Simon Horman <ho...@verge.net.au> wrote:
> On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:
>> On Mon, May 20, 2013 at 5:55 PM, Simon Horman <ho...@verge.net.au> wrote:
>> > On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:
>> >> On Fri, May 17, 2013 at 12:06 AM, Simon Horman <ho...@verge.net.au> wrote:
>> >> > +static int push_mpls(struct sk_buff *skb,
>> >> > + const struct ovs_action_push_mpls *mpls)
>> >> > +{
>> >> > + __be32 *new_mpls_lse;
>> >> > + int err;
>> >> > +
>> >> > + err = make_writable(skb, skb->mac_len + MPLS_HLEN);
>> >> > + if (unlikely(err))
>> >> > + return err;
>> >> > +
>> >> > + skb_push(skb, MPLS_HLEN);
>> >>
>> >> What happens if there isn't enough headroom?
>> >
>> > Good point. How about this?
>> >
>> > if (unlikely(skb->data<skb->head))
>> > return -EINVAL;
>> > skb_push(skb, MPLS_HLEN);
>>
>> The amount of headroom is an internal kernel property so we can't
>> reject actions on the basis of it. We need to expand the headroom,
>> similar to __vlan_put_tag().
>
> Thanks. I have changed the code around and it is now as follows.
> I am a little unsure if the combination of make_writable() and
> skb_cow_head() is sensible.
>
> err = make_writable(skb, skb->mac_len + MPLS_HLEN);
> if (unlikely(err))
> return err;
>
> if (skb_cow_head(skb, MPLS_HLEN) < 0)
> return -ENOMEM;
>
> skb_push(skb, MPLS_HLEN);
I would reverse the order of make_writable() and skb_cow_head() to
avoid possibly having to reallocate twice. I think you also don't need
to add MPLS_HLEN to the length in make_writable() because that
function is ensuring that existing packet data can be modified and the
MPLS label isn't there yet.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
