[ovs-dev] [PATCH v3 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

Thanks for you comments.

On Fri, Jul 8, 2016 at 10:35 PM, Eric Garver <e at erig.me> wrote:
> Hi Xiao,
>
> Thanks for the series. One patch comment below.
>
> General comments:
>
>         1) Various places use VLAN_CFI to indicate presence of a VLAN tag
>            (many checks for tci != 0, because CFI bit set). Since we're now
>            tracking the TPID would it be more appropriate to instead use the
>            TPID to indicate presence of a VLAN tag?

Often a flow matches VLAN tag and don't care about the exact TPID,
then we can set the mask bit of VLAN_CFI and wildcard TPID. I think
when indicating the presence of VLAN, it's more suitable to use CFI
bit, following OpenFlow standard. In this patch TPID and CFI are used
interchangeably, I will go through clean it up.

>
>         2) eth_type_vlan() checks only 0x8100 and 0x88a8. Older switch
>            implementations used 0x9100 for outer tags. It may be worth
>            supporting 0x9100 as well even though OF v1.1 only specifies
>            0x8100 and 0x88a8.

Many vendor specific ethertypes (0x9100, 0x9200, ...) were used before
standard and should be deprecated. Supporting them is possible at this
time, but may have problems in the future (these ethertypes may be
used for other purposes). And it also requires changes in kernel,
which is not likely.

>
> Thanks.
> Eric.
>
> On Sun, Jul 03, 2016 at 08:47:06AM +0800, Xiao Liang wrote:
>> diff --git a/tests/test-classifier.c b/tests/test-classifier.c
>> index c74c440..1826936 100644
>> --- a/tests/test-classifier.c
>> +++ b/tests/test-classifier.c
>> @@ -1702,7 +1702,8 @@ test_miniflow(struct ovs_cmdl_context *ctx OVS_UNUSED)
>>          miniflow = miniflow_create(&flow);
>>
>>          /* Check that the flow equals its miniflow. */
>> -        assert(miniflow_get_vid(miniflow) == vlan_tci_to_vid(flow.vlan_tci));
>> +        assert(miniflow_get_vid(miniflow) ==
>> +               vlan_tci_to_vid(flow.vlan[0].tci));
>>          for (i = 0; i < FLOW_U64S; i++) {
>>              assert(miniflow_get(miniflow, i) == flow_u64[i]);
>>          }
>
> This only checks the outer VLAN. Should it also check the inner?

You are right, should also check inner VLAN here.

Thanks,
Xiao