|WARNING| [v3,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/167049

_AI Code Review_

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

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

---

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

### Errors

**rte_pcapng_add_interface: NULL check after use**
In `lib/pcapng/rte_pcapng.c`, line 280 adds a NULL check for `self`:
```c
if (self == NULL)
    return -EINVAL;
```
However, this check comes *after* line 284 dereferences `self` in `rte_eth_dev_info_get()`:
```c
ret = rte_eth_dev_info_get(port, &dev_info);
```
Wait, looking more carefully: the NULL check is at line 280, before the call at 284. The logic is correct here. No issue.

**rte_pcapng_write_stats: stats validation logic error**
At line 416-417:
```c
if (stats == NULL || stats_sz == 0 || stats_sz > sizeof(*stats))
    return -EINVAL;
```
The condition `stats_sz > sizeof(*stats)` is correct for rejecting oversized input. However, the comment at line 421 says "Future proof for more/less stats" and the code uses `memcpy(&isb, stats, stats_sz)` to handle a caller passing a *smaller* structure (older version). But the validation rejects `stats_sz == 0`. Since the comment says "Must be greater than 0", this is intentional, not a bug. The check is correct.

Actually, re-reading: the validation allows `stats_sz` to be less than `sizeof(*stats)` (for forward compatibility) but not zero. That makes sense. The upper bound check also makes sense: a caller passing `stats_sz > sizeof(*stats)` would overflow the `memcpy`. The bounds are correct.

**Missing error check propagation in rte_pcapng_add_interface**
Line 285:
```c
ret = rte_eth_dev_info_get(port, &dev_info);
if (ret < 0)
    return -1;  /* should be ret */
```
The comment says "should be ret". This is correct: the function should propagate the actual error code from `rte_eth_dev_info_get()`, not hardcode `-1`. This loses the specific errno that the device info query returned.

### Warnings

**Release notes placement**
The patch modifies `doc/guides/rel_notes/release_26_11.rst` to document the API change. The target release is stated as `26_11` in the patch metadata (date 2026-07-30). The release notes correctly describe the change under both "API Changes" and "ABI Changes" sections. This is correct.

---

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

### Errors

**Resource leak on error path in capture_alloc**
In `lib/capture/capture.c`, the `capture_alloc()` function allocates several resources (ring, mempool, filter, output string) but the error path at line 750 only calls `capture_free(cap)`. Let's trace through `capture_free()` at line 663:
- Checks if `cap == NULL` and returns early if so
- Frees `cap->output` (strdup'd string)
- Calls `__rte_capture_filter_free(cap->filter)`
- Calls `rte_ring_free(cap->ring)`
- Calls `rte_mempool_free(cap->mp)`
- Calls `rte_free(cap)`

This looks correct: all resources allocated in `capture_alloc()` before the error are cleaned up by `capture_free()`. The `goto error` path is safe.

**Use-after-free risk in capture_cb_detach handshake**
The comment at line 177-182 explains the handshake between detaching a callback and waiting for the datapath to release it. The pattern is:
1. Store NULL to `cbs->cap` (line 179)
2. Fence seq_cst (line 182)
3. Wait for `use_count` to be even (line 185)

The datapath (lines 192-195, 201-202) does:
1. Increment `use_count` (odd)
2. Fence seq_cst (line 195)
3. Load `cbs->cap` (line 301/322)
4. Increment `use_count` (even) when done

This is a correct use of seq_cst fencing to ensure visibility. At least one side sees the other's operation. No use-after-free can occur. The RTE_WAIT_UNTIL_MASKED ensures the wait completes. Correct.

**Missing NULL check before dereference**
In `capture_attach()` at line 369:
```c
cbs = capture_cb_get(cap, queue, is_rx);
if (cbs == NULL) {
    CAPTURE_LOG(ERR, "No callback block for %u:%u %s", ...);
    return -1;
}
```
The NULL check is present. No issue.

**Telemetry parameter parsing: unchecked strdup**
In `capture_alloc()` at line 736:
```c
cap->output = strdup(cfg->output);
if (cap->output == NULL) {
    capture_err(d, "Could not strdup '%s'", cfg->output);
    goto error;
}
```
The `strdup()` result is checked. Correct.

**FIFO handling: blocking open**
In `capture_pcapng_open()` at line 624:
```c
*fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK);
```
Then at line 638-640, the code switches the fd back to blocking mode. The comment at line 619-622 explains this is intentional: non-blocking open prevents hanging on a FIFO without a reader, then blocking mode ensures the capture thread blocks on write if the reader is slow. This is a correct pattern for FIFO handling.

**Stale callback handling**
At lines 377-379:
```c
rte_free((void *)(uintptr_t)cbs->stale);
cbs->stale = NULL;
```
And at lines 507-508, the callback is marked stale:
```c
cbs->stale = cbs->cb;
cbs->cb = NULL;
```
The comment at lines 372-376 explains that ethdev reads `cb->next` after the callback returns, so the callback object cannot be freed at removal time. Deferring it until the next attach is safe. This is correct.

**Error path in capture_start_req: callbacks not removed**
At line 1069 (`error_callback_remove` label), `capture_remove_callbacks(cap)` is called, which detaches all callbacks. But if `capture_add_callbacks(cap)` partially succeeds (installs some callbacks then fails on a later queue), the earlier callbacks remain installed. Let's check `capture_add_callbacks()` at line 414:
- It installs Tx callbacks for all queues (or selected queue)
- Then installs Rx callbacks for all queues (or selected queue)
- Returns -1 on first failure

If it fails partway through, the callbacks installed so far are in `cap->cbs[]` with `cap->nb_cbs` tracking the count. The `capture_remove_callbacks()` at line 478 iterates `for (uint16_t i = 0; i < cap->nb_cbs; i++)` and removes each one. So partial installation is correctly cleaned up. No leak.

**Potential race in capture_sum_stats**
At line 467, `capture_sum_stats()` is called with `capture_lock` held, and it reads `cbs->stats` for each attached callback. The stats are updated by the datapath without holding `capture_lock`. However, each individual stat field is a uint64_t and the loads/stores should be atomic on 64-bit platforms. The worst case is reading a slightly stale or torn value during a concurrent update, which is acceptable for statistics. The comment at line 485 says this is the "final counters before letting go", which is correct because the callback is detached before the lock is taken again. No race condition with consequences.

Wait, actually: at line 463, `capture_sum_stats(cap, &total)` is called inside the loop that detaches callbacks, and at line 492 it's called again after detaching all. Line 493 takes the lock. So the first call at line 463 is **outside** the lock, reading stats while the datapath may still be updating them. But this is safe because:
1. The stats fields are uint64_t, which are atomic on 64-bit
2. The detach handshake ensures no datapath thread is actively running the callback
3. The final sum at line 492 happens after all callbacks are detached and before the blocks go on the free list

Actually, the call at line 463 is *not* in the patch. Let me re-check the line numbers. The patch shows:
```c
static void
capture_remove_callbacks(struct capture *cap)
{
    struct capture_total total;

    for (uint16_t i = 0; i < cap->nb_cbs; i++) {
        ...
        capture_cb_detach(cbs);
    }

    rte_spinlock_lock(&capture_lock);
    capture_sum_stats(cap, &total);
    cap->total = total;
    ...
    rte_spinlock_unlock(&capture_lock);
}
```
So the sum is done *after* detaching all callbacks and *inside* the lock. The detach waits for use_count to be even, so no datapath thread is touching the stats. This is correct.

**telemetry command truncation**
The telemetry library limits strings to 1024 bytes per the code comment at line 85 ("CAP_CMD_MAX 1024"). The check at line 1001:
```c
if (strlcpy(tmp, params, CAP_CMD_MAX) >= CAP_CMD_MAX) {
    CAPTURE_LOG(ERR, "params too long");
    return -1;
}
```
This is correct: `strlcpy` returns the length it *would* have written (not including the null terminator), so `>= CAP_CMD_MAX` means truncation occurred.

**NULL pointer dereference: tel_cmd/capture_accepted**
In several places (e.g., line 165-168 in test_capture.c), the code calls `tel_cmd()` which sends a command over `tel_sock`. If `tel_sock` is -1, `send()` will fail with errno EBADF. The caller checks the return value, so this is handled. No dereference of a NULL pointer.

### Warnings

**Global atomic counter in get_unique_id**
At line 655:
```c
static unsigned int
get_unique_id(void)
{
    static RTE_ATOMIC(unsigned int) capture_instance;

    return rte_atomic_fetch_add_explicit(&capture_instance, 1, rte_memory_order_relaxed);
}
```
This is correct: a relaxed fetch-add for a counter that only needs to be unique (no ordering dependency). Well done.

**Release notes**
The release notes are updated correctly for the new library. Documentation is added under `doc/guides/prog_guide/capture_lib.rst`. This is correct.

**Installed headers**
The patch adds a new library under `lib/capture/` with no public API headers (it's driven entirely through telemetry). The `capture_impl.h` header is not listed in `headers =` in the meson.build, so it won't be installed. This is correct.

**Use of bool**
The code uses `bool` for the `is_rx` field in `struct capture_rxtx_cb` and for the `running` flag in `struct capture`. This is correct and preferred over `int`.

### Info

**Naming convention**
The library uses `capture_` prefix for static functions and `rte_capture_` for the logging type. There are no exported symbols (no public API). This is correct.

**Documentation**
The documentation in `doc/guides/prog_guide/capture_lib.rst` is thorough and matches the implementation. Well done.

---

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

### Errors

**Potential fd leak in test_capture_dual**
At line 580:
```c
fd2 = open(path2, O_CREAT | O_WRONLY, 0600);
TEST_ASSERT(fd2 >= 0, "could not create %s: %s", path2, strerror(errno));
close(fd2);
```
The fd is closed immediately after creation. The test then starts a capture that opens the same path. If the capture fails to start, the file is unlinked at line 602. No leak. Correct.

**Missing error check on inject_rx/inject_tx**
The test cases call `inject_rx()` and `inject_tx()` which return -1 on failure. All call sites check the return value with `TEST_ASSERT_SUCCESS()`. Correct.

**telemetry client: socket not closed on error**
In `tel_connect()` at line 118:
```c
if (connect(s, ...) < 0) {
    close(s);
    return -1;
}
```
And at line 124:
```c
if (recv(s, ...) <= 0) {
    close(s);
    return -1;
}
```
Both error paths close the socket. Correct.

### Warnings

**Test registration**
The test is registered with `REGISTER_FAST_TEST(capture_autotest, NOHUGE_OK, ASAN_OK, test_capture);`. This is the correct current format.

---

## PATCH 4/4: usertools: add script for wireshark capture

### Errors

**comma injection in telemetry parameters**
The `comma_error()` function at line 236 and the checks at lines 312-315 correctly reject commas in the fifo path and filter string. Since the telemetry protocol is comma-separated with no escaping, a comma in a value would be mis-parsed. This is correct validation.

**Exception handling in cmd_interfaces**
At line 164, the code catches `OSError` when connecting to telemetry and logs it to stderr. The loop continues to the next prefix. This is correct: one unreachable primary doesn't block discovering others.

**File descriptor leak in cmd_capture**
At line 298, `fifo_fd = os.open(fifo, os.O_WRONLY)` is called. If the telemetry connection fails at line 302, the code calls `os.close(fifo_fd)` at line 304 before raising. If `tel.command(cmd)` raises at line 316, the code closes `fifo_fd` at line 318 and `tel.close()` at line 319. If the capture start returns an error at line 323, the same cleanup happens at lines 324-325. All error paths close the fd. Correct.

### Warnings

**Signal handling in wait_for_stop**
The function sets up signal handlers and a wakeup fd, then polls both the FIFO fd and the wakeup pipe. The old handlers and wakeup fd are restored in the finally block at lines 272-276. This is correct and ensures cleanup even if an exception occurs.

**Use of strtoull without error checking**
At lines 160 and 168, `strtoull()` is called without checking for conversion errors. However, both are preceded by a check that the string starts with a digit (lines 156-157, 167). The return value is used even if the conversion is partial (e.g., "123abc" -> 123). For an interface list query this is acceptable because a malformed telemetry reply is not a critical path. A stricter check would be better, but this is not a correctness bug.

---

## Summary

### Errors to Fix

**PATCH 1/4:**
1. `rte_pcapng_add_interface()` at line 286: should propagate `ret` instead of returning `-1` on error.

### Warnings

**PATCH 2/4:**
1. Consider adding a compile-time assert that `sizeof(struct capture_rxtx_cb)` is a multiple of `RTE_CACHE_LINE_SIZE` to catch unexpected padding.

### Overall Assessment

The patch series is of high quality:
- No use-after-free or resource leaks were found
- Error paths correctly clean up allocated resources
- Synchronization between datapath and control path is correctly implemented with seq_cst fencing
- Telemetry parameter validation correctly rejects unsafe input
- The test suite provides good coverage of the core functionality
- Documentation is thorough and matches the implementation

The only correctness issue