[dpdk-dev] [PATCH v3] app/testpmd: fix testpmd packets dump overlapping

[Jiawei(Jonny) Wang]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at intel.com>
> Sent: Saturday, November 21, 2020 1:50 AM
> To: Jiawei(Jonny) Wang <jiaweiw at nvidia.com>; wenzhuo.lu at intel.com;
> beilei.xing at intel.com; bernard.iremonger at intel.com; Ori Kam
> <orika at nvidia.com>; Slava Ovsiienko <viacheslavo at nvidia.com>; NBU-
> Contact-Thomas Monjalon <thomas at monjalon.net>; Raslan Darawsheh
> <rasland at nvidia.com>
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v3] app/testpmd: fix testpmd packets dump overlapping
> 
> On 11/20/2020 5:35 PM, Jiawei Wang wrote:
> > When testpmd enabled the verbosity for the received packets, if two
> > packets were received at the same time, for example, sampling packet
> > and normal packet, the dump output of these packets may be overlapping
> > due to multiple core handling the multiple queues simultaneously.
> >
> > The patch uses one string buffer that collects all the packet dump
> > output into this buffer and then printouts it at last, that guarantees
> > to printout separately the dump output per packet.
> >
> > Fixes: d862c45 ("app/testpmd: move dumping packets to a separate
> > function")
> >
> > Signed-off-by: Jiawei Wang <jiaweiw at nvidia.com>
> 
> <...>
> 
> > @@ -74,13 +85,16 @@
> >   	uint32_t vx_vni;
> >   	const char *reason;
> >   	int dynf_index;
> > +	int buf_size = MAX_STRING_LEN;
> > +	char print_buf[buf_size];
> > +	int cur_len = 0;
> >
> > +	memset(print_buf, 0, sizeof(print_buf));
> 
> Should 'print_buf' cleaned per each packet below, if not can we drop 'memset'
> completely?
> 
Since the 'snprintf' always append the a terminating null character('\0') after the written string,
and in the code the following character be appended start from previous null character, so only one 
'\0' will be appended in the last 'snprintf' calls, 
	snprintf(buf + cur_len, buf_size - cur_len ..
At the last 'printf("%s") will print the string buffer up to a terminating null character ('\0').
so it's ok even not 'memset' calls to clean 'print_buf' per each packets. 
> <...>
> 
> > +		if (cur_len >= buf_size) {
> > +			printf("%s ...\n", print_buf);
> > +			break;
> 
> Why break here? Wouldn't just append some chars to indicate trancation and
> continue be OK?
> 
> 
Yes, doesn't  need 'break'  here.