RE: [PATCH] app/testpmd: fix VLAN header parsing

["Morten Brørup" <mb@smartsharesystems.com>]

[-- Attachment #1: Type: text/plain, Size: 2785 bytes --]

Comment inline below.

 

From: Raslan Darawsheh [mailto:rasland@nvidia.com] 
Sent: Monday, 24 March 2025 08.35

 

>> From: Raslan Darawsheh [mailto:rasland@nvidia.com]
>> Sent: Sunday, 23 March 2025 13.28
>> 
>> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN
>> headers.
>> 
>> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API")
>> Cc: haijie1@huawei.com
>> 
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 5b906eaa53..302cc4cc66 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>  {
>>        struct rte_vlan_hdr *vlan_hdr;
>>        uint16_t ethertype;
>> +     uint32_t i = 0;
>> 
>>        switch (ptype) {
>>        case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, uint32_t ptype)
>>                return _htons(RTE_ETHER_TYPE_IPV6);
>>        default:
>>                ethertype = eth_hdr->ether_type;
>> -             while (eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> -                     eth_hdr->ether_type == _htons(RTE_ETHER_TYPE_QINQ)) {
>> +             while (ethertype == _htons(RTE_ETHER_TYPE_VLAN) ||
>> +                     ethertype == _htons(RTE_ETHER_TYPE_QINQ)) {
>>                        vlan_hdr = (struct rte_vlan_hdr *)
>> -                             ((char *)eth_hdr + sizeof(*eth_hdr));
>> +                             ((char *)eth_hdr + sizeof(*eth_hdr) +
>> +                             (i * sizeof(struct rte_vlan_hdr)));

>Doesn't work; "i" is not incremented, and remains 0.
>Instead do this: Initialize (struct rte_vlan_hdr *)vlan_hdr=RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) before the loop, and move vlan_hdr+=RTE_VLAN_HLEN inside the loop.
Yes you are correct I had a (i++ *) but must have slipped while rebasing will handle your way in v2 

MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving it should be vlan_hdr++, not vlan_hdr+=RTE_VLAN_HLEN.

MB: And you might want to impose a maximum boundary to the loop, as suggested by Stephen. E.g. break out of the loop if vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked VLAN tags should suffice.


>>                        ethertype = vlan_hdr->eth_proto;
>>                }
>>                return ethertype;
>> --
>> 2.39.5 (Apple Git-154)

 

Kindest regards

Raslan Darawsheh


[-- Attachment #2: Type: text/html, Size: 8292 bytes --]