|WARNING| [v2,4/4] usertools: add script for wireshark capture

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/166956

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-29

I'll review this DPDK patch series against the guidelines in AGENTS.md, focusing on correctness bugs, C coding style, API requirements, and other violations.

# Review of DPDK Patch Series v2

## Patch 1/4: pcapng: extend interface statistics

### Errors

**stats_sz validation allows zero**
In `rte_pcapng_write_stats()` at lib/pcapng/rte_pcapng.c:420, the validation:
```c
if (stats == NULL || stats_sz == 0 || stats_sz > sizeof(*stats))
    return -EINVAL;
```
rejects `stats_sz == 0`, but the comment in the header says "Must be greater than 0". The code is consistent with the documentation, so this is correct. However, immediately after this check, the code does:
```c
memset(&isb, 0xff, sizeof(isb));
memcpy(&isb, stats, stats_sz);
```
If `stats_sz == 0` were allowed (by removing it from the check), the memcpy would be a no-op but the memset would still initialize the structure to all-1s (UINT64_MAX sentinel). The current rejection of zero is correct.

**NULL pointer check after validation that can never trigger**
At lib/pcapng/rte_pcapng.c:397-399:
```c
if (self == NULL)
    return -EINVAL;
```
This NULL check is good practice.

At lib/pcapng/rte_pcapng.c:411:
```c
if (self == NULL)
    return -EINVAL;
```
Another NULL check, also good.

At lib/pcapng/rte_pcapng.c:740-743:
```c
if (self == NULL) {
    rte_errno = EINVAL;
    return -1;
}
```
A third NULL check in `rte_pcapng_write_packets()`. All three are consistent defensive programming.

### Info

The patch changes the API signature of `rte_pcapng_write_stats()` from individual counters to a structure pointer with size parameter. This is well-documented in both the release notes (API Changes and ABI Changes sections) and the header file. The approach of passing size allows future expansion of the statistics structure while maintaining ABI compatibility, which is good design.

The structure initialization pattern (memset to 0xff, then memcpy the provided data) is a clever way to handle forward/backward compatibility: fields not provided by the caller default to UINT64_MAX (the "unavailable" sentinel), and fields added in the future that the caller doesn't know about are also defaulted to UINT64_MAX.

---

## Patch 2/4: capture: infrastructure wireshark packet capture

### Errors

**Resource leak on error path in capture_alloc()**
At lib/capture/capture.c:758-773, `capture_alloc()` allocates several resources in sequence (cap structure, ring, mempool, filter, output string). The error path at line 773 calls `capture_free()`, which correctly releases all of them. However, examining the allocation sequence:

1. Line 764: `cap = rte_zmalloc_socket(...)` - freed by `capture_free(cap)` at line 774
2. Line 772: `cap->ring = rte_ring_create(...)` - freed by `rte_ring_free(cap->ring)` in `capture_free()`
3. Line 785: `cap->mp = rte_pktmbuf_pool_create_by_ops(...)` - freed by `rte_mempool_free(cap->mp)` in `capture_free()`
4. Line 793: `cap->filter = __rte_capture_filter_create(...)` - freed by `__rte_capture_filter_free(cap->filter)` in `capture_free()`
5. Line 800: `cap->output = strdup(...)` - freed by `free(cap->output)` in `capture_free()`

All error paths after each allocation correctly jump to `error:` which calls `capture_free()`. The `capture_free()` function at lines 721-730 correctly handles NULL pointers for each field (rte_ring_free, rte_mempool_free, and the filter function all accept NULL; free(cap->output) is safe because `cap->output` is initialized to NULL by rte_zmalloc_socket). No leak here.

**Statistics accumulation using = instead of +=**
At lib/capture/capture.c:851-854:
```c
isb.ifrecv = t.accepted + t.filtered + t.nombuf;
isb.filteraccept = t.accepted + t.nombuf;
isb.ifdrop = t.nombuf + t.ringfull;
```
These are assignments into a local structure `isb` that is then passed to `rte_pcapng_write_stats()`. The values in `t` (struct capture_total) are already the accumulated totals (computed by `capture_sum_stats()` at line 849). This is not incremental accumulation of statistics; it's computing interface statistics from capture statistics for the pcapng block. The `=` operator is correct here.

**Double-close risk in capture_pcapng_open()**
At lib/capture/capture.c:688-722, on the error path at line 717:
```c
rte_pcapng_close(pcapng);  /* closes fd */
pcapng = NULL;
```
The comment states that `rte_pcapng_close()` closes the fd, so the subsequent `goto cleanup` at line 719 does not need to close it again. However, there's a `goto close_fd` branch at lines 710 and 713 that falls through to:
```c
close_fd:
    close(*fd);
cleanup:
    free(osname);
    free(ifdescr);
    return pcapng;
```
The issue: if pcapng is non-NULL but we take the `goto close_fd` path (lines 710 or 713), we call `close(*fd)` even though the pcapng object might have taken ownership of it. Let me trace the paths:

- Line 688: fd is an out-parameter (`int *fd`)
- Line 706: `*fd = open(...)` - we own the fd
- Line 714: `pcapng = rte_pcapng_fdopen(*fd, ...)` - pcapng takes ownership of the fd
- Line 715: if `pcapng == NULL`, jump to `close_fd` and close the fd. But wait - did `rte_pcapng_fdopen()` take ownership even though it failed?

Checking rte_pcapng API documentation would clarify this. If `rte_pcapng_fdopen()` takes ownership even on failure (which would be unusual), then the `goto close_fd` at line 716 would double-close. If it does NOT take ownership on failure (the usual contract), then the code is correct. Given that the function explicitly calls `rte_pcapng_close(pcapng)` in the success-then-fail case (lines 720-722), which "closes fd", I believe the contract is that fdopen DOES take ownership, which means the `goto close_fd` at line 716 would be a bug.

However, looking more carefully: line 715 checks `if (pcapng == NULL)`, which means fdopen FAILED. The standard contract for "fdopen" functions is that they do NOT take ownership on failure. So the `goto close_fd` at line 716 is correct. The "closes fd" comment on line 717 is about the SUCCESS case where we later fail at line 720.

Actually, let me re-read the flow:
```c
714:	pcapng = rte_pcapng_fdopen(*fd, osname, NULL, appname, NULL);
715:	if (pcapng == NULL) {
716:		CAPTURE_LOG(ERR, "Add section block failed");
717:		goto close_fd;
718:	}
719:
720:	if (rte_pcapng_add_interface(pcapng, port_id, DLT_EN10MB, ifname, ifdescr, filter) < 0) {
721:		CAPTURE_LOG(ERR, "Add interface for port %u:%s failed", port_id, ifname);
722:		rte_pcapng_close(pcapng);  /* closes fd */
723:		pcapng = NULL;
724:	}
725:	goto cleanup;
```
Line 725 skips the `close_fd:` label, so there's no double-close risk here. The paths are:
- fdopen succeeds, add_interface succeeds - line 725 goto cleanup, no close
- fdopen succeeds, add_interface fails - line 722 rte_pcapng_close, line 725 goto cleanup, no further close
- fdopen fails - line 717 goto close_fd, then cleanup

This is correct. No bug here.

**Use-after-free risk in capture callback lifecycle**
The capture_rxtx_cb structure (lib/capture/capture_impl.h:110-123) is never freed. The comment explains: "allocated on demand and never freed... Keeping this block alive for the life of the process gives that thread something valid to synchronize on." The synchronization protocol between `capture_cb_detach()` (lines 175-185) and `capture_cb_hold()`/`capture_cb_release()` (lines 188-203) uses atomic operations and a memory fence. Let me verify the protocol:

The handshake is:
- Detach (control plane): store cap=NULL (relaxed), fence (seq_cst), wait for use_count to be even (acquire)
- Hold (data plane): increment use_count (relaxed), fence (seq_cst), load cap (acquire)
- Release (data plane): increment use_count (release)

The seq_cst fences ensure that either the data plane sees cap=NULL (and skips the capture), or the control plane sees an odd use_count (and waits). Once use_count is observed to be even, no data plane thread can be inside the critical section that dereferences cap. This protocol is correct.

However, there's a subtlety in the reuse of callback blocks. At line 388-392 of capture.c:
```c
/* the block may have been used by an earlier capture of this queue */
memset(&cbs->stats, 0, sizeof(cbs->stats));

/*
 * Release the callback object left over by that earlier capture...
```
The comment acknowledges reuse. The `stale` field (capture_impl.h:121) holds the old ethdev callback pointer, which is freed at line 395:
```c
rte_free((void *)(uintptr_t)cbs->stale);
```
The logic is that when a callback block is reused, the previous capture's ethdev callback pointer is still referenced by ethdev's callback list (because `rte_eth_remove_rx_callback()` only unlinks, doesn't free), so we can't free it until the NEXT capture of the same queue. This deferred-free pattern is correct given ethdev's API constraints. No bug here.

**Unbounded descriptor chain traversal**
No descriptor chain traversals are present in this library. Not applicable.

**Bitmask shift issues**
No bit shifts on masks are present. Not applicable.

**MTU vs frame length confusion**
Not applicable to this library.

**Missing error checks**
At lib/capture/capture.c:1128, the return value of `rte_thread_create()` is checked. Good.

At line 905, the return value of `fcntl()` is checked. Good.

At line 868, `uname()` return is checked. The comment says "OS name is optional, just keep going if not found", which is correct - `asprintf()` failure also just leaves `osname` as NULL, which is then checked at line 907 before use. Good.

At line 873, the return value of `rte_eth_dev_get_name_by_port()` is checked. Good.

Overall, error handling looks thorough.

### Warnings

**Function return type not on own line**
At lib/capture/capture_impl.h:23-24:
```c
struct rte_capture_filter *__rte_capture_filter_create(const char *str);
```
The return type `struct rte_capture_filter *` is on the same line as the function name. Per guidelines, it should be on its own line. However, this is a header file declaration, not a definition. The guidelines specifically state "Function Format" with an example of the **definition**. Declarations in headers commonly have the return type on the same line and that's idiomatic C. This is acceptable practice for declarations.

Checking the definitions:
- lib/capture/filter.c:30-31:
```c
struct rte_capture_filter *
__rte_capture_filter_create(const char *filter)
```
Correct: return type on its own line.

- lib/capture/filter.c:90-91:
```c
const char *__rte_capture_filter_string(struct rte_capture_filter *filter)
```
Return type on the same line as function name. This is a definition and should have the return type on its own line per guidelines.

- lib/capture/filter.c:95-96:
```c
void __rte_capture_filter_free(struct rte_capture_filter *filter)
```
Same issue.

- lib/capture/filter.c:104-105:
```c
uint64_t __rte_capture_filter(const struct rte_capture_filter *filter, struct rte_mbuf *mb)
```
Same issue.

These four function definitions in filter.c violate the style guideline.

**Global variable naming**
At lib/capture/capture.c:67-68:
```c
TAILQ_HEAD(capture_list, capture);
static struct capture_list capture_list = TAILQ_HEAD_INITIALIZER(capture_list);
static rte_spinlock_t capture_lock = RTE_SPINLOCK_INITIALIZER;
```
The `capture_list` variable and `capture_lock` are static, so they don't need the `rte_` or `RTE_` prefix (that's only for external symbols). However, the `capture_cb_freelist` at line 133 has a more descriptive name. For consistency and clarity, `capture_list` could be `capture_active_list`, but this is minor.

At lib/capture/capture.c:133:
```c
static struct capture_cb_list capture_cb_freelist =
```
Good naming.

**Missing release notes for new library**
The release notes at doc/guides/rel_notes/release_26_11.rst:58-60 state:
```
* **Added wireshark capture support.**

  * Added ``capture`` library for packet capture via telemetry API.
```
This is present in the "New Features" section. Good.

**No REGISTER_FAST_TEST for the new library**
Patch 3/4 adds app/test/test_capture.c, which provides test coverage. This is correct separation.

**Release notes document API that doesn't exist**
The capture library has no public C API - it's driven entirely through telemetry. The release note correctly describes this as "via telemetry API". No issue.

**Export macros**
The capture library has no public functions, so no `RTE_EXPORT_*` macros are needed. The filter functions are internal (prefixed with `__rte_`) and not exported. Correct.

### Info

The architecture is sound: the library uses ethdev Rx/Tx callbacks to copy packets into a ring, and a dedicated thread drains the ring into a pcapng file or FIFO. The synchronization protocol for callback lifecycle (never freeing the callback blocks, using atomic use_count and cap pointer with seq_cst fences) correctly handles the race between callback removal and datapath threads still executing in the callback.

The filter implementation uses libpcap to compile the filter expression to eBPF and then runs it via rte_bpf. This is a clean integration.

The telemetry-driven API is unconventional but appropriate for this use case: it allows capture to be started on a running application without source changes or EAL arguments. The documentation in doc/guides/prog_guide/capture_lib.rst is thorough and clear.

---

## Patch 3/4: test: add test for capture hooks

### Errors

None identified. The test is well-structured: it sets up a net_null vdev, opens a FIFO, connects to the telemetry socket, and exercises the capture lifecycle. The use of `wait_list_empty()` to poll for capture teardown is appropriate given the asynchronous nature of the drain thread. The test cases cover the key scenarios: lifecycle, bad queue rejection, queue selection, and concurrent captures.

### Warnings

**Missing assert message improvement**
Several assertions could have more descriptive messages. For