|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 patch series for DPDK coding standards compliance and potential issues.

commit 5a478bee46e88-2ba9c484e5aso8641865eec
Author: Stephen Hemminger <stephen at networkplumber.org>

net/af_packet: fix indentation

DPDK code is supposed to be indented with TABS not spaces.
Also, used "unsigned int" to keep checkpatch happy.

> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 0ee94e71ea..8303ff5ca9 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c

[ ... ]

> @@ -890,7 +890,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  	if (pair == NULL) {
>  		PMD_LOG(ERR,
>  			"%s: no interface specified for AF_PACKET ethdev",
> -		        name);
> +			name);

Does this line still have a mix of tabs and spaces, or is it now purely
tabs? The coding standard requires pure tabs for indentation.

> @@ -1226,7 +1226,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
>  	if (framesize > blocksize) {
>  		PMD_LOG(ERR,
>  			"%s: AF_PACKET MMAP frame size exceeds block size!",
> -		        name);
> +			name);

Same concern - is the indentation now pure tabs?


commit 5a478bee46e88-2e2e4f21fa8mr213470eec
Author: Stephen Hemminger <stephen at networkplumber.org>

test: add test for af_packet

> 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
> @@ -0,0 +1,1122 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2026 Stephen Hemminger
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <linux/if.h>
> +#include <linux/if_tun.h>
> +#include <linux/sockios.h>
> +#include <pthread.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
> +#include <time.h>
> +#include <unistd.h>

The coding style guide states system headers should be included before
RTE headers. Can the ordering be verified for compliance?

> +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;
> +	}

Does this code properly handle the case where open() succeeds but later
operations fail? If ioctl() or socket() fails, does fd leak?

> +	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 follow C99 style requirements? The coding standard requires
all declarations at the start of the block. Does sock need to be
declared at the function entry?

> +	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);
> +	}

If socket() fails (sock < 0), does fd leak?

> +static unsigned int
> +alloc_tx_mbufs(struct rte_mbuf **bufs, unsigned int count)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	ret = rte_pktmbuf_alloc_bulk(mp, bufs, count);
> +	if (ret != 0)
> +		return 0;
> +
> +	for (i = 0; i < count; i++) {
> +		struct rte_ether_hdr *eth_hdr;
> +		eth_hdr = (struct rte_ether_hdr *)rte_pktmbuf_append(bufs[i],
> +				sizeof(struct rte_ether_hdr) + 46);
> +		if (eth_hdr == NULL) {
> +			/* Free all allocated mbufs on failure */
> +			rte_pktmbuf_free_bulk(bufs, count);
> +			return 0;
> +		}

If rte_pktmbuf_append() fails when i > 0, does this code correctly free
only the mbufs that were allocated? Should it free i mbufs instead of
count?

> +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;
> +	}

Does this function properly clean up mp if later operations fail? What
happens if create_tap_interface() fails - does mp leak?

> +	/* 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 */
> +	}

When returning 0 here, is mp properly freed or does it leak until
teardown?

> +	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;
> +	}
> +	port_started = true;

When these failures occur, does the cleanup path properly handle all the
resources allocated so far (mp, tap_fd, port)?

> +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];

Does this 2048-byte stack allocation create any portability concerns?

> +	/* Set TAP fd to non-blocking for reading */
> +	int flags = fcntl(tap_fd, F_GETFL, 0);

Does this follow C99 style? Should flags be declared at function entry?

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

If fcntl(F_GETFL) fails and returns -1, does this code properly handle
the error case?

> +	/* 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);

What happens if write() returns an error? Does this properly handle
short writes or errors?

> +static int
> +test_af_packet_multi_queue(void)
> +{

[ ... ]

> +	allocated = alloc_tx_mbufs(bufs, BURST_SIZE / 2);
> +		if (allocated > 0)
> +			do_tx_burst(test_port, q, bufs, allocated);

Does the indentation look correct here? Should the if statement be
aligned with the for loop?

> +static void *
> +mt_reader_thread(void *arg)
> +{
> +	struct mt_test_ctx *ctx = arg;
> +	struct rte_mbuf *bufs[BURST_SIZE];
> +	uint16_t nb_rx;
> +
> +	while (!rte_atomic_load_explicit(&ctx->stop, rte_memory_order_relaxed)) {
> +		nb_rx = rte_eth_rx_burst(ctx->port_id, 0, bufs, BURST_SIZE);
> +		if (nb_rx > 0) {
> +			ctx->rx_count += nb_rx;

Is ctx->rx_count properly protected for concurrent access? The reader
thread writes it and the main thread reads it - does this need atomic
operations?