|SUCCESS| [v4,2/2] test: add test for af_packet

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Reviewing the AF_PACKET PMD test patch:

Git SHA: Not provided in patch header
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v4 2/2] test: add test for af_packet

This patch adds a comprehensive test suite for the AF_PACKET PMD,
including device info, link status, TX/RX, loopback, promiscuous mode,
MAC address, MTU, stats, multi-queue, and multi-threaded tests.

> diff --git a/app/test/test_pmd_af_packet.c b/app/test/test_pmd_af_packet.c
> new file mode 100644
> index 0000000000..abd60fe18c
> --- /dev/null
> +++ b/app/test/test_pmd_af_packet.c

[ ... ]

> +/*
> + * Create a TAP interface for testing.
> + * Returns fd on success, -1 on failure.
> + */
> +static int
> +create_tap_interface(const char *name)
> +{
> +	struct ifreq ifr;
> +	int fd, ret;
> +
> +	fd = open("/dev/net/tun", O_RDWR);
> +	if (fd < 0) {
> +		printf("Cannot open /dev/net/tun: %s\n", strerror(errno));
> +		printf("(Are you running as root?)\n");
> +		return -1;
> +	}
> +
> +	memset(&ifr, 0, sizeof(ifr));
> +	ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
> +	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", name);
> +
> +	ret = ioctl(fd, TUNSETIFF, &ifr);
> +	if (ret < 0) {
> +		printf("Cannot create TAP interface '%s': %s\n",
> +		       name, strerror(errno));
> +		close(fd);
> +		return -1;
> +	}
> +
> +	/* Bring the interface up */
> +	int sock = socket(AF_INET, SOCK_DGRAM, 0);

Does this code leak the socket on allocation failure? The function
returns -1 without closing sock when socket() succeeds but later
operations fail. If socket() returns >= 0, should sock be closed
before returning -1?

> +	if (sock >= 0) {
> +		memset(&ifr, 0, sizeof(ifr));
> +		snprintf(ifr.ifr_name, IFNAMSIZ, "%s", name);
> +
> +		/* Get current flags */
> +		if (ioctl(sock, SIOCGIFFLAGS, &ifr) == 0) {
> +			ifr.ifr_flags |= IFF_UP;
> +			ioctl(sock, SIOCSIFFLAGS, &ifr);
> +		}
> +		close(sock);
> +	}
> +
> +	printf("Created TAP interface '%s'\n", name);
> +	return fd;
> +}

[ ... ]

> +static int
> +create_af_packet_port(const char *name, const char *args, uint16_t *out_port_id)
> +{
> +	int ret;
> +
> +	ret = rte_vdev_init(name, args);
> +	if (ret != 0) {
> +		printf("Failed to create af_packet device '%s': %d\n", name, ret);
> +		return ret;
> +	}
> +
> +	ret = rte_eth_dev_get_port_by_name(name, out_port_id);
> +	if (ret != 0) {
> +		printf("Failed to get port id for '%s': %d\n", name, ret);
> +		rte_vdev_uninit(name);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int
> +test_af_packet_setup(void)
> +{
> +	char devargs[256];
> +	int ret;
> +
> +	/* Create mempool for mbufs */
> +	mp = rte_pktmbuf_pool_create("af_packet_test_pool", NUM_MBUFS,
> +				     MBUF_CACHE_SIZE, 0,
> +				     RTE_MBUF_DEFAULT_BUF_SIZE,
> +				     rte_socket_id());
> +	if (mp == NULL) {
> +		printf("Failed to create mempool\n");
> +		return -1;
> +	}
> +
> +	/* Create TAP interface for testing */
> +	tap_fd = create_tap_interface(TAP_DEV_NAME);
> +	if (tap_fd >= 0)
> +		tap_created = true;
> +	else {
> +		printf("TAP interface creation failed - tests will be skipped\n");
> +		return 0; /* Return success to allow skipped tests */
> +	}
> +
> +	/* Create and configure af_packet port */
> +	snprintf(devargs, sizeof(devargs), "iface=%s", TAP_DEV_NAME);
> +	ret = create_af_packet_port(AF_PACKET_DEV_NAME, devargs, &port_id);
> +	if (ret != 0) {
> +		printf("Failed to create af_packet port\n");
> +		return -1;
> +	}
> +	port_created = true;
> +
> +	ret = configure_af_packet_port(port_id, 1, 1);
> +	if (ret != 0) {
> +		printf("Failed to configure af_packet port\n");
> +		return -1;
> +	}

Does test_af_packet_setup() leak resources when configure_af_packet_port()
fails? When returning -1 here, mp, tap_fd, and the created vdev port are
not cleaned up. Should test_af_packet_teardown() be called or should cleanup
be performed before returning -1?

> +	port_started = true;
> +
> +	return 0;
> +}

[ ... ]

> +/*
> + * Test: Loopback - TX packets and receive them back via TAP interface.
> + * This exercises both the TX and RX paths in a round-trip.
> + */
> +static int
> +test_af_packet_loopback(void)
> +{
> +	struct rte_mbuf *tx_bufs[BURST_SIZE];
> +	struct rte_mbuf *rx_bufs[BURST_SIZE];
> +	uint16_t nb_tx, total_rx = 0;
> +	uint64_t elapsed = 0;
> +	unsigned int allocated;
> +	char tap_buf[2048];
> +
> +	if (!tap_created || port_id >= RTE_MAX_ETHPORTS) {
> +		printf("SKIPPED: Port not available (need root)\n");
> +		return TEST_SKIPPED;
> +	}
> +
> +	/* Set TAP fd to non-blocking for reading */
> +	int flags = fcntl(tap_fd, F_GETFL, 0);

Does test_af_packet_loopback() check the return value of fcntl()? If
fcntl(tap_fd, F_GETFL, 0) returns -1, flags contains -1, and the next
fcntl() call will use invalid flags. Should the return value be checked
before using flags?

> +	fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK);

[ ... ]

> +	/*
> +	 * Read packets from TAP interface (they were sent there by af_packet).
> +	 * Then write them back to TAP so af_packet can receive them.
> +	 */
> +	while (elapsed < LOOPBACK_TIMEOUT_US) {
> +		ssize_t bytes_read, bytes_written;
> +
> +		/* Read from TAP (what af_packet sent) */
> +		bytes_read = read(tap_fd, tap_buf, sizeof(tap_buf));
> +		if (bytes_read > 0) {
> +			/* Write back to TAP for af_packet to receive */
> +			bytes_written = write(tap_fd, tap_buf, bytes_read);
> +			TEST_ASSERT(bytes_read == bytes_written, "TAP write %zd != %zd",
> +				    bytes_read, bytes_written);

Does test_af_packet_loopback() check if write() succeeds? If write()
returns -1, bytes_written will be -1, which is different from bytes_read,
but the error condition is not explicitly handled. Should write() failure
be checked before comparing bytes?

> +		}
> +
> +		/* Try to receive via af_packet RX */
> +		uint16_t nb_rx = rte_eth_rx_burst(port_id, 0, rx_bufs, BURST_SIZE);
> +		if (nb_rx > 0) {
> +			total_rx += nb_rx;
> +			rte_pktmbuf_free_bulk(rx_bufs, nb_rx);
> +		}
> +
> +		/* Check if we've received enough */
> +		if (total_rx >= nb_tx)
> +			break;
> +
> +		rte_delay_us_block(STATS_POLL_INTERVAL_US);
> +		elapsed += STATS_POLL_INTERVAL_US;
> +	}
> +
> +	/* Restore TAP fd flags */
> +	fcntl(tap_fd, F_SETFL, flags);

[ ... ]

> +/*
> + * Multi-threaded test context
> + */
> +struct mt_test_ctx {
> +	uint16_t port_id;
> +	RTE_ATOMIC(uint32_t) stop;
> +	uint32_t rx_count;

Does struct mt_test_ctx need rx_count to be atomic? The rx_count field is
modified in mt_reader_thread() and read in test_af_packet_multithread()
without atomic operations. Should rx_count use RTE_ATOMIC(uint32_t) to
prevent data races?

> +};

[ ... ]