|SUCCESS| [v24, 24/24] test: add comprehensive test suite for pcap PMD

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163159

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-16

I'll review this comprehensive DPDK pcap PMD patch series focusing on correctness, coding style, API compliance, and guideline violations.

commit 2b843cac232e ("drivers: use per line logging in helpers")
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: fix build on Windows

The log messages in the pcap OS dependent code for Windows was never
converted during the last release.

[ ... ]

> diff --git a/drivers/net/pcap/pcap_osdep_windows.c b/drivers/net/pcap/pcap_osdep_windows.c
> index 1d398dc7ed..0965c2f5c9 100644
> --- a/drivers/net/pcap/pcap_osdep_windows.c
> +++ b/drivers/net/pcap/pcap_osdep_windows.c
> @@ -53,7 +53,7 @@ osdep_iface_index_get(const char *device_name)
>  
>  	ret = GetAdapterIndex(adapter_name, &index);
>  	if (ret != NO_ERROR) {
> -		PMD_LOG(ERR, "GetAdapterIndex(%S) = %lu\n", adapter_name, ret);
> +		PMD_LOG(ERR, "GetAdapterIndex(%S) = %lu", adapter_name, ret);

Does this code properly handle the wide string format specifier %S?
Windows adapter_name is a WCHAR*, so verify this printf format is
portable across compilers. Consider using %ls if needed.

> @@ -75,20 +75,20 @@ osdep_iface_mac_get(const char *device_name, struct rte_ether_addr *mac)
>  
>  	sys_ret = GetAdaptersAddresses(AF_UNSPEC, 0, NULL, NULL, &size);
>  	if (sys_ret != ERROR_BUFFER_OVERFLOW) {
> -		PMD_LOG(ERR, "GetAdapterAddresses() = %lu, expected %lu\n",
> +		PMD_LOG(ERR, "GetAdapterAddresses() = %lu, expected %lu",
>  			sys_ret, ERROR_BUFFER_OVERFLOW);

The code uses ULONG for sys_ret but ERROR_BUFFER_OVERFLOW might not be
unsigned long on all platforms. Does this format string match the type
properly? Consider casting or using PRIu32 if needed.

---

commit (include used headers)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: include used headers

[ ... ]

> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index f323c0b0df..4513d46d61 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -4,16 +4,23 @@
>   * All rights reserved.
>   */
>  
> +#include <stdio.h>
>  #include <stdlib.h>
>  #include <time.h>
> -
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/time.h>
> +#include <sys/types.h>

Does this code need sys/types.h explicitly? On POSIX systems it's often
transitively included. Verify Windows compatibility of sys/time.h since
the series claims Windows support.

---

commit (avoid using rte_malloc and rte_memcpy)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: avoid using rte_malloc and rte_memcpy

[ ... ]

> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index d6fe062b92..724b7a9038 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -1287,7 +1288,7 @@ eth_pcap_update_mac(const char *if_name, struct rte_eth_dev *eth_dev,
>  		return -1;
>  
>  	PMD_LOG(INFO, "Setting phy MAC for %s", if_name);
> -	rte_memcpy(mac_addrs, mac.addr_bytes, RTE_ETHER_ADDR_LEN);
> +	memcpy(mac_addrs, mac.addr_bytes, RTE_ETHER_ADDR_LEN);

This change mixes standard library usage with DPDK APIs. Does this
affect performance on architectures where rte_memcpy provides
optimizations? Verify that removing rte_memcpy doesn't regress on
platforms with specialized implementations.

---

commit (replace stack bounce buffer)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: replace stack bounce buffer

[ ... ]

> @@ -966,7 +964,7 @@ static int
>  eth_tx_queue_setup(struct rte_eth_dev *dev,
>  		uint16_t tx_queue_id,
>  		uint16_t nb_tx_desc __rte_unused,
> -		unsigned int socket_id __rte_unused,
> +		unsigned int socket_id,
>  		const struct rte_eth_txconf *tx_conf __rte_unused)
>  {
>  	struct pmd_internals *internals = dev->data->dev_private;
> @@ -974,11 +972,26 @@ eth_tx_queue_setup(struct rte_eth_dev *dev,
>  
>  	pcap_q->port_id = dev->data->port_id;
>  	pcap_q->queue_id = tx_queue_id;
> +	pcap_q->bounce_buf = rte_malloc_socket(NULL, RTE_ETH_PCAP_SNAPSHOT_LEN,
> +					       RTE_CACHE_LINE_SIZE, socket_id);
> +	if (pcap_q->bounce_buf == NULL)
> +		return -ENOMEM;

Does this code properly clean up bounce_buf on port close or queue
setup failure? The tx_queue_release callback added later should handle
this, but verify no leak if setup fails after this allocation.

---

commit (fix error accounting and backpressure)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: fix error accounting and backpressure on transmit

[ ... ]

> @@ -484,26 +494,27 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  
>  	for (i = 0; i < nb_pkts; i++) {
>  		struct rte_mbuf *mbuf = bufs[i];
> -		size_t len = rte_pktmbuf_pkt_len(mbuf);
> -		int ret;
> +		uint32_t len = rte_pktmbuf_pkt_len(mbuf);
> +		const uint8_t *data;
>  
>  		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
>  				len > RTE_ETH_PCAP_SNAPSHOT_LEN)) {
>  			PMD_LOG(ERR,
> -				"Dropping multi segment PCAP packet. Size (%zd) > max size (%u).",
> +				"Dropping multi segment PCAP packet. Size (%u) > max size (%u).",
>  				len, RTE_ETH_PCAP_SNAPSHOT_LEN);

Does this code leak mbuf when dropping oversized packets? The
rte_pktmbuf_free(mbuf) is missing before the continue statement.

> +			tx_queue->tx_stat.err_pkts++;
>  			rte_pktmbuf_free(mbuf);
>  			continue;
>  		}
>  
> -		/* rte_pktmbuf_read() returns a pointer to the data directly
> -		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
> -		 * a pointer to temp_data after copying into it.
> -		 */
> -		ret = pcap_sendpacket(pcap,
> -			rte_pktmbuf_read(mbuf, 0, len, temp_data), len);
> -		if (unlikely(ret != 0))
> +		data = rte_pktmbuf_read(mbuf, 0, len, temp_data);
> +		RTE_ASSERT(data != NULL);
> +
> +		if (unlikely(pcap_sendpacket(pcap, data, len) != 0)) {
> +			/* Assume failure is backpressure */
> +			PMD_LOG(ERR, "pcap_sendpacket() failed: %s", pcap_geterr(pcap));
>  			break;
> +		}

Does this error message spam the log under normal backpressure? Consider
rate-limiting this log or downgrading to DEBUG level for transient errors.

---

commit (add link status for interface mode)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: add link status for interface mode

[ ... ]

> +int
> +osdep_iface_link_status(const char *name)
> +{
> +	struct ifmediareq ifmr;
> +	int fd, status = 0;
> +
> +	fd = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (fd == -1)
> +		return -1;
> +
> +	memset(&ifmr, 0, sizeof(ifmr));
> +	strlcpy(ifmr.ifm_name, if_name, sizeof(ifmr.ifm_name));

Does this code use the correct parameter name? The function parameter is
'name' but strlcpy uses 'if_name'. This will fail to compile.

---

commit (add snaplen devarg)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: add snapshot length devarg

[ ... ]

> +static int
> +process_snapshot_len(const char *key, const char *value, void *extra_args)
> +{
> +	uint32_t *snaplen = extra_args;
> +	unsigned long val;
> +	char *endptr;
> +
> +	if (value == NULL || *value == '\0') {
> +		PMD_LOG(ERR, "Argument '%s' requires a value", key);
> +		return -1;
> +	}
> +
> +	errno = 0;
> +	val = strtoul(value, &endptr, 10);
> +	if (errno != 0 || *endptr != '\0' ||
> +	    val < RTE_ETHER_HDR_LEN ||
> +	    val > ETH_PCAP_MAXIMUM_SNAPLEN) {

Does this code handle leading whitespace properly? strtoul skips leading
whitespace but the check against ETH_PCAP_MAXIMUM_SNAPLEN might not match
user expectations if the string contains trailing spaces that endptr points to.

---

commit (add Rx scatter offload)
Author: Stephen Hemminger <stephen at networkplumber.org>

net/pcap: add Rx scatter offload

[ ... ]

> @@ -1117,11 +1128,37 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
>  {
>  	struct pmd_internals *internals = dev->data->dev_private;
>  	struct pcap_rx_queue *pcap_q = &internals->rx_queue[rx_queue_id];
> +	uint16_t buf_size;
> +	bool rx_scatter;
> +
> +	buf_size = rte_pktmbuf_data_room_size(mb_pool) - RTE_PKTMBUF_HEADROOM;
> +	rx_scatter = !!(dev->data->dev_conf.rxmode.offloads &
> +			RTE_ETH_RX_OFFLOAD_SCATTER);
> +
> +	/*
> +	 * If Rx scatter is not enabled, verify that the mbuf data room
> +	 * can hold the largest received packet in a single segment.
> +	 * Use the MTU-derived frame size as the expected maximum, not
> +	 * snapshot_len which is a capture truncation limit rather than
> +	 * an expected packet size.
> +	 */
> +	if (!rx_scatter) {
> +		uint32_t max_rx_pktlen = dev->data->mtu + RTE_ETHER_HDR_LEN;
> +
> +		if (max_rx_pktlen > buf_size) {
> +			PMD_LOG(ERR,
> +				"Rx scatter is disabled and RxQ mbuf pool object size is too small "
> +				"(buf_size=%u, max_rx_pkt_len=%u)",
> +				buf_size, max_rx_pktlen);
> +			return -EINVAL;
> +		}
> +	}

Does this code properly handle the default MTU value at queue setup time?
If the application hasn't explicitly set MTU, does dev->data->mtu contain
a valid value or might it be uninitialized?

---

commit (add comprehensive test suite)
Author: Stephen Hemminger <stephen at networkplumber.org>

test: add comprehensive test suite for pcap PMD

[ ... ]

> +static int
> +create_temp_path(char *buf, size_t buflen, const char *prefix)
> +{
> +	char temp_dir[MAX_PATH];
> +	char temp_file[MAX_PATH];
> +	DWORD ret;
> +
> +	ret = GetTempPathA(sizeof(temp_dir), temp_dir);
> +	if (ret == 0 || ret > sizeof(temp_dir))
> +		return -1;

Does this code handle the case where ret == sizeof(temp_dir)? The Windows
API documentation states that if the return value equals the buffer size,
the path may be truncated.

> +	if (GetTempFileNameA(temp_dir, prefix, 0, temp_file) == 0)
> +		return -1;
> +
> +	ret = snprintf(buf, buflen, "%s.pcap", temp_file);
> +	if (ret >= buflen) {
> +		DeleteFileA(temp_file);
> +		return -1;
> +	}

Does this code create a race condition? GetTempFileNameA creates the file,
but then it's renamed. Between DeleteFileA(temp_file) and the rename,
another process could create a file with the target name.

> +/*
> + * Helper: Allocate a multi-segment mbuf with controlled segment size.
> + */
> +static struct rte_mbuf *
> +alloc_multiseg_mbuf(uint32_t pkt_len, uint16_t seg_size, uint8_t fill_byte)
> +{
> +	struct rte_mbuf *head = NULL;
> +	struct rte_mbuf **prev = &head;
> +	uint32_t remaining = pkt_len;
> +	uint16_t nb_segs = 0;
> +
> +	while (remaining > 0) {
> +		struct rte_mbuf *seg = rte_pktmbuf_alloc(mp);
> +		uint16_t this_len;
> +
> +		if (seg == NULL) {
> +			rte_pktmbuf_free(head);
> +			return NULL;
> +		}

Does this code properly free all allocated segments on failure? When
seg allocation fails, calling r