Hi Stephen,
> -----Original Message-----
> From: Stephen Hemminger <step...@networkplumber.org>
> Sent: Wednesday, January 11, 2023 9:37 PM
> To: Amit Prakash Shukla <amitpraka...@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Kiran Kumar
> Kokkilagadda <kirankum...@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpu...@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 <amitpraka...@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;
> > + }
