[ovs-dev] [PATCH] datapath: Fix pop_vlan()

[Pravin Shelar]

On Mon, Nov 14, 2011 at 3:57 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Fri, Nov 11, 2011 at 4:23 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> diff --git a/acinclude.m4 b/acinclude.m4
>> index 648132a..458b9e5 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -238,6 +238,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>>   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_warn_if_lro],
>>                   [OVS_DEFINE([HAVE_SKB_WARN_LRO])])
>>   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [consume_skb])
>> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_reset_mac_len])
>
> Usually we don't add tests unless it's known to be backported.  In
> this case I'm guessing (although you should check) that it can be
> nested under HAVE_SKBUFF_HEADER_HELPERS.
It can not be nested in HAVE_SKBUFF_HEADER_HELPERS.
I will add kernel version check.

>
>> @@ -254,6 +255,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>>   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_get_be16])
>>   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_find_nested])
>>
>> +  OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [vlan_set_encap_proto])
>
> Since we know that this isn't backported, I would just leave out the
> test for now and then we can put a version check in when we move this
> to be non-static upstream.
>
ok

>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index ac7187b..f9746ff 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static void vlan_reorder_header(struct sk_buff *skb)
>> +{
>
> It's not really good to have a function with the same name as another
> one in a different file.  I would normally expect that callers of this
> function are actually using the one in vlan_core.c but they're not and
> this one is subtly different.
>
ok

>>  static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>>  {
> [...]
>> +       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> +       *current_tci = vhdr->h_vlan_TCI;
>
> It would actually be better to just use vlan_eth_hdr().
>
ok

>> +       vlan_set_encap_proto(skb, vhdr);
>>
>> -       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> +       skb_pull_rcsum(skb, VLAN_HLEN);
>> +       vlan_reorder_header(skb);
>
> These functions don't do the right thing because they assume that the
> current position of skb->data is different from what it actually is at
> this point in OVS.  They assume that the Ethernet header has already
> been pulled off but we pushed it back on, so the offsets are wrong.
>

right, I need to use VLAN_ETH_HLEN.

>> +       skb_set_network_header(skb, VLAN_HLEN);
>> +       skb_set_transport_header(skb, VLAN_HLEN);
>
> These offsets aren't right - the network header assumes that we have
> no Ethernet header on at the moment and the transport header will be
> the same as the network header when in reality it should be based on
> our previous parsing.
>
>> diff --git a/datapath/vlan.h b/datapath/vlan.h
>> index 7e1084e..1374f43 100644
>> --- a/datapath/vlan.h
>> +++ b/datapath/vlan.h
>> +#ifndef HAVE_VLAN_SET_ENCAP_PROTO
>> +void vlan_set_encap_proto(struct sk_buff *skb, struct vlan_hdr *vhdr);
>> +#endif
>
> This doesn't really belong in this file.  Since it is a straight
> backport without any dependencies on other code, it belongs in the
> compat directory.
>

ok