On Mon, Jun 6, 2016 at 7:51 PM, Simon Horman <simon.hor...@netronome.com> wrote:
> On Thu, Jun 02, 2016 at 03:02:00PM -0700, pravin shelar wrote:
>> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
>> <simon.hor...@netronome.com> wrote:
>> > Allow push and pop mpls actions to act on layer 3 packets by teaching
>> > them not to access non-existent L2 headers of such packets.
>> >
>> > Signed-off-by: Simon Horman <simon.hor...@netronome.com>
>> > ---
>> > v10
>> > * Limit scope of hdr in {push,pop}_mpls()
>> >
>> > v9
>> > * New Patch
>> > ---
>> > net/openvswitch/actions.c | 19 ++++++++++++-------
>> > 1 file changed, 12 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> > index 9a3eb7a0ebf4..15f130e4c22b 100644
>> > --- a/net/openvswitch/actions.c
>> > +++ b/net/openvswitch/actions.c
>> > @@ -172,7 +172,8 @@ static int push_mpls(struct sk_buff *skb, struct
>> > sw_flow_key *key,
>> >
>> > skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
>> >
>> > - update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
>> > + if (skb->mac_len)
>> > + update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
>> We can move all ethernet related code in this if block. for example
>> memmove().
>
> My assumption is that the memmove() does nothing if skb->mac_len is zero
> and from my point of view it seems clean to leave it where it is unless
> the code around it also moves.
>
> Is there other code you think could/should be moved into the
> if (skb->mac_len) block?
>
yes, the code is correct. But I think grouping l2 related updated
together makes code easier to follow.
