RE: [EXT] Re: [PATCH v2 2/3] graph: pcap capture for graph nodes

[Amit Prakash Shukla <amitprakashs@marvell.com>]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, January 11, 2023 9:37 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Kiran Kumar
> Kokkilagadda <kirankumark@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; dev@dpdk.org
> Subject: [EXT] Re: [PATCH v2 2/3] graph: pcap capture for graph nodes
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 11 Jan 2023 14:23:41 +0530
> Amit Prakash Shukla <amitprakashs@marvell.com> wrote:
> 
> > +
> > +#define PCAP_DUMP_DATA(dbuf, buf_size, cur_len, sbuf, len)                   \
> > +do {                                                                         \
> > +	if ((cur_len + len) >= buf_size)                                     \
> > +		break;                                                       \
> > +	rte_memcpy(dbuf + cur_len, sbuf, len);                               \
> > +	cur_len += len;                                                      \
> > +} while (0)
> > +
> 
> Why do you need this to be a macro.
> Macro's are evil, have side effects and hide code.

I had added macro for future, if lot of custom data is to be added to pcapng. Anyways I will remove it
in next version of patch.

> 
> > +uint16_t
> > +rte_graph_pcap_trace_dispatch(struct rte_graph *graph __rte_unused,
> > +			      struct rte_node *node, void **objs,
> > +			      uint16_t nb_objs)
> > +{
> > +	uint64_t i, num_packets;
> > +	struct rte_mbuf *mbuf_clones[RTE_GRAPH_BURST_SIZE] = { };
> > +	char buffer[GRAPH_PCAP_BUF_SZ] = {0};
> 
> The initialization probably is not needed here.
> 
> Couldn't you just do:
> 	rte_strlcpy(buffer, node->name, GRAPH_PCAP_BUF_SZ);
> 
> > +	for (i = 0; i < num_packets; i++) {
> > +		struct rte_mbuf *mc;
> > +		mbuf = (struct rte_mbuf *)objs[i];
> > +
> > +		mc = rte_pcapng_copy(port_id, 0, mbuf, mp, mbuf->pkt_len,
> > +				     rte_get_tsc_cycles(), 0, buffer);
> > +		if (mc == NULL)
> > +			goto done;
> 
> The code will leak mbuf's if pcapng_copy() fails.
> Suppose packet #2 caused the pool to get exhausted.
> That copy would fail, but the mbuf for packets 0 and 1 would already be
> sitting in mbuf_clones.

My bad. Thanks for catching the issue. I will correct it in next version of the patch.

> > +
> > +		mbuf_clones[i] = mc;
> > +	}