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

[Stephen Hemminger <stephen@networkplumber.org>]

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.

> +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.
> +
> +		mbuf_clones[i] = mc;
> +	}