Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: move dumping packets to a separate function

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/16/2018 11:10 AM, Raslan Darawsheh wrote:
> Hi Ferruh,
> 
> PSB.
> 
> Kindest regards,
> Raslan Darawsheh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, October 11, 2018 5:56 PM
>> To: Raslan Darawsheh <rasland@mellanox.com>; jingjing.wu@intel.com
>> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Shahaf
>> Shuler <shahafs@mellanox.com>; Xueming(Steven) Li
>> <xuemingl@mellanox.com>; Ori Kam <orika@mellanox.com>;
>> jerin.jacob@caviumnetworks.com; david.marchand@6wind.com;
>> bernard.iremonger@intel.com
>> Subject: Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: move dumping
>> packets to a separate function
>>
>> On 10/7/2018 8:38 AM, Raslan Darawsheh wrote:
>>> verbosity for the received/sent packets is needed in all of the
>>> forwarding engines so moving it to be in a separate function
>>
>> +1, this is good idea.
>>
>>> ---
>>> changes in v3:
>>> 	- add util.c in the mason.build file
>>> 	- restore missing check for ol_flags & PKT_RX_RSS_HASH.
>>> 	- add local variables for rte_be_to_cpu to avoid long
>>> 	  lines.
>>>
>>> changes in v4:
>>> 	- add missing l3 and l4 checks
>>> ---
>>>
>>> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
>>
>> <...>
>>
>>> @@ -0,0 +1,150 @@
>>> +/* SPDX-License-Identifie.r: BSD-3-Clause
>>> + * Copyri.ght(c) 2010-2018 Mellanox technology.
>>
>> It can be good to keep original Copyright owner when moving some code
>> from one file another.
> I added a new copy right since it contains some new functionality as well not only the original one.

What is the new functionality added to "dump_pkt_burst()" ? Overall there is a
new feature but not into this file.
I think better to keep the original copyright when you are copying something
from one file to another and append your copyright when added a significant
contribution to that file.

>>
>> <...>
>>
>>> +		sw_packet_type = rte_net_get_ptype(mb, &hdr_lens,
>>> +						   RTE_PTYPE_ALL_MASK);
>>
>> DPDK coding convention requires a single tab in next line, instead of alligning
>> to the parenthesis.
> Will send a new version with these fixed.
>>
>> <...>
>>
>>> +			/* Do not support ipv4 option field */
>>> +			if (RTE_ETH_IS_IPV4_HDR(packet_type)) {
>>> +				l3_len = sizeof(struct ipv4_hdr);
>>> +				ipv4_hdr = rte_pktmbuf_mtod_offset(mb,
>>> +				struct ipv4_hdr *,
>>> +				l2_len);
>>> +				l4_proto = ipv4_hdr->next_proto_id;
>>
>> The syntax broken here.
> This is the same as present in rxonly file nothing changed
>>
>> <...>
>>
>>> +		}
>>> +		printf(" - %s queue=0x%x", is_rx ? "Receive" : "Send",
>>> +			(unsigned int) queue);
>>
>> Isn't this same information with initial header line, does this add any
>> extra/new information?
> 
> This is as of per packet, since you can get more than one packet printed when you have RSS the first one will be a print for the entire burst,
> and then you'll get this print for each packet printed.
>