|WARNING| [v4,3/3] usertools: add script for wireshark capture

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-13

# DPDK Patch Review

## Summary

This patch series introduces a new packet capture library for DPDK that enables live traffic capture via telemetry API, along with Wireshark integration through an extcap plugin.

---

## ERRORS

### Patch 1/3: lib/capture/capture.c

**Error 1: Resource leak on error path after capture_link()**
*Line 380-384 in capture.c:*
```c
uint32_t idx = cap->idx;
capture_link(cap);

rte_thread_t thread_id;
int ret = rte_thread_create(&thread_id, NULL, capture_thread, cap);
if (ret != 0) {
    capture_err(d, "thread start failed: %s", strerror(ret));
    goto error_unlink;
}
```
After `capture_link(cap)` adds the capture to the global list, if `rte_thread_create()` fails, the code jumps to `error_unlink` which calls `capture_mp_broadcast(cap, CAPTURE_MP_STOP)`. If that broadcast fails (returns `-1`), the code logs an error but returns success (`return 0` at line 408), leaking the capture structure that remains linked in `capture_list`. The drain thread never ran to unlink it, and the cleanup path is skipped.

**Fix:** Change line 408 from `return 0;` to `goto error_callback_remove;` so that both the unlink and the full cleanup happen even when the broadcast fails:
```c
error_unlink:
    if (capture_mp_broadcast(cap, CAPTURE_MP_STOP) < 0) {
        CAPTURE_LOG(ERR,
                    "capture %u: secondary still attached, leaking ring and pool",
                    cap->idx);
        capture_unlink(cap);
        capture_remove_callbacks(cap);
        /* fall through to free the capture struct */
    } else {
        capture_unlink(cap);
    }
error_callback_remove:
    capture_remove_callbacks(cap);
    capture_free(cap);
    return 0;
```

---

**Error 2: Statistics accumulation using `=` instead of `+=`**
*Line 371 in datapath.c:*
```c
static void
capture_sum_one(struct capture_total *t, const struct capture_stats *s)
{
    t->accepted += s->accepted;
    t->filtered += s->filtered;
    t->nombuf   += s->nombuf;
    t->ringfull += s->ringfull;
}
```
This function correctly uses `+=`, but the pattern must be verified everywhere statistics are accumulated. After manual inspection of all call sites (`capture_sum_stats`, `capture_detach_queues`), the usage is correct. **Retracted** -- no error here.

---

**Error 3: MTU-related code in a capture library**
The capture library does not appear to handle MTU or scattered Rx, so the MTU-related rules from the guidelines do not apply here. **Not applicable.**

---

### Patch 1/3: lib/capture/output.c

**Error 4: File descriptor leak on capture_pcapng_open() failure path**
*Lines 92-110 in output.c:*
```c
if (fcntl(*fd, F_SETFL, flags & ~O_NONBLOCK) < 0) {
    CAPTURE_LOG(ERR, "fcntl %s: %s",  path,  strerror(errno));
    goto close_fd;
}

pcapng = rte_pcapng_fdopen(*fd, osname, NULL, appname, NULL);
if (pcapng == NULL) {
    CAPTURE_LOG(ERR, "Add section block failed");
    goto close_fd;
}

if (rte_pcapng_add_interface(pcapng, port_id, DLT_EN10MB, ifname, ifdescr, filter) < 0) {
    CAPTURE_LOG(ERR, "Add interface for port %u:%s failed", port_id, ifname);
    rte_pcapng_close(pcapng);  /* closes fd */
    pcapng = NULL;
}
goto cleanup;

close_fd:
close(*fd);
```
If `rte_pcapng_add_interface()` fails (line 104), the code calls `rte_pcapng_close(pcapng)` which closes `*fd`, then jumps to `cleanup` which frees `osname` and `ifdescr`. That path is correct. However, if `rte_pcapng_fdopen()` fails (line 99), the code jumps to `close_fd:` which calls `close(*fd)`, but `*fd` was already passed to `rte_pcapng_fdopen()`. If `rte_pcapng_fdopen()` takes ownership of the fd on entry (even on failure), this is a double-close. If it does not, the close is correct.

Inspection needed: does `rte_pcapng_fdopen()` take ownership of the fd on failure? Without reading `lib/pcapng/rte_pcapng.c`, this cannot be determined. **Flag as potential double-close** -- the caller should verify that `rte_pcapng_fdopen()` does not close the fd on failure, or adjust the error path.

**Likely correct** if `rte_pcapng_fdopen()` does not close on failure, but the pattern is fragile. The safe pattern is:
```c
pcapng = rte_pcapng_fdopen(*fd, osname, NULL, appname, NULL);
if (pcapng == NULL) {
    CAPTURE_LOG(ERR, "Add section block failed");
    goto close_fd;  /* only if fdopen does NOT close on failure */
}
```
Or if `fdopen` closes on failure:
```c
pcapng = rte_pcapng_fdopen(*fd, osname, NULL, appname, NULL);
if (pcapng == NULL) {
    CAPTURE_LOG(ERR, "Add section block failed");
    *fd = -1;  /* prevent double-close */
    goto cleanup;
}
```
**Recommendation:** Document the ownership in a comment, or check the pcapng API.

---

### Patch 1/3: lib/capture/multiproc.c

**Error 5: Missing check on rte_mp_action_register() return**
*Lines 177-181 in multiproc.c:*
```c
void
capture_mp_register(void)
{
    if (rte_mp_action_register(CAPTURE_MP, capture_mp_handler) < 0 &&
        rte_errno != ENOTSUP)
        CAPTURE_LOG(NOTICE, "Could not register ipc action: %s",
                    rte_strerror(rte_errno));
}
```
If `rte_mp_action_register()` fails with an error other than `ENOTSUP`, a log is emitted but the function returns successfully. Later, when `capture_mp_broadcast()` is called, it will fail because no handler is registered. The capture will proceed without multi-process support, which may be acceptable, but the error is silent.

**Not an error** -- this is defensive logging. The code assumes single-process is a valid fallback. The guidelines do not require failing here. **Retracted.**

---

### Patch 2/3: app/test/test_capture.c

**Error 6: Use of deprecated REGISTER_FAST_TEST macro**
*Line 688 in test_capture.c:*
```c
REGISTER_FAST_TEST(capture_autotest, NOHUGE_OK, ASAN_OK, test_capture);
```
The guidelines state:
> Do NOT flag `REGISTER_FAST_TEST` using `NOHUGE_OK`/`ASAN_OK` macros (this is the correct current format).

**Retracted** -- this is correct.

---

### Patch 3/3: usertools/dpdk-wireshark-extcap.c

**Error 7: Privilege not dropped before opening FIFO in cmd_capture()**
*Lines 930-937 in dpdk-wireshark-extcap.c:*
```c
fifo_fd = open(fifo, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
if (fifo_fd < 0)
    err(1, "cannot open fifo %s", fifo);
if (fstat(fifo_fd, &sb) != 0 || !S_ISFIFO(sb.st_mode))
    errx(1, "%s is not a fifo", fifo);

if (tel_handshake(&t, fd) < 0)
    err(1, "no banner from DPDK telemetry");
```
The FIFO open is the first thing `cmd_capture()` does, before `tel_handshake()`. The telemetry handshake reads the banner unprivileged (per the comment at line 363: "Runs unprivileged: this is the first point at which the primary gets to send us anything"). But the FIFO open in `cmd_capture()` happens *before* that, so if `cmd_capture()` is called while still privileged, the FIFO is opened privileged.

Looking at `main()`: `priv_drop()` is called at line 1107, then `cmd_capture()` is invoked at line 1121. So the FIFO open is **unprivileged**. **Retracted** -- the drop happens before capture.

---

### Patch 3/3: usertools/dpdk-wireshark-extcap.c

**Error 8: Missing validation of privileged_exec in cmd_capture()**
*Lines 917-925 in dpdk-wireshark-extcap.c:*
```c
if (o.capture) {
    struct stat sb;

    if (o.fifo == NULL)
        errx(1, "--capture requires --fifo");
    why = param_problem(o.fifo);
    if (why != NULL)
        errx(1, "fifo path %s", why);
```
The code comment at line 102 states:
> After a privileged start the plugin is more restrictive: the environment is ignored, only `/var/run/dpdk` is searched, and the output must be a FIFO.

However, the check `if (!S_ISFIFO(sb.st_mode))` in `cmd_capture()` at line 937 enforces the FIFO requirement unconditionally, not just after a privileged start. The guidelines do not require different behavior here, and the FIFO check is security-correct in both cases. **Not an error** -- the restriction is documented, not a bug.

---

## WARNINGS

### Patch 1/3: lib/capture/meson.build

**Warning 1: Missing release notes for internal API**
*Lines 19-23 in meson.build:*
```
if dpdk_conf.has('RTE_HAS_LIBPCAP')
    sources += files('filter.c')
    ext_deps += pcap_dep
else
    warning('libpcap is missing, capture filtering will be disabled')
endif
```
The release notes in `doc/guides/rel_notes/release_26_11.rst` document the new library but do not mention the optional `libpcap` dependency or the reduced functionality when built without it. This is not a public API, but the operational difference is significant.

**Recommendation:** Add a note in the release notes or the library documentation about the `libpcap` dependency.

---

### Patch 1/3: lib/capture/datapath.c

**Warning 2: Static function pointer array not const**
*Lines 31-32 in private.h:*
```c
static struct capture_cb_list capture_cb_freelist =
    TAILQ_HEAD_INITIALIZER(capture_cb_freelist);
```
This is a TAILQ head, not a function pointer array. **Not applicable.**

---

### Patch 1/3: Documentation

**Warning 3: RST documentation uses bullet lists instead of definition lists**
*Lines 55-64 in doc/guides/prog_guide/capture_lib.rst:*
```rst
``/ethdev/capture/start`` takes the port id as its first parameter, followed by
``name=value`` options separated by commas:

``out=<path>``
   Required. Path of the output, which must already exist and be a FIFO.

``snaplen=<n>``
   Number of bytes to capture from each packet. Default is 262144;
   ``snaplen=0`` means capture the whole packet.
```
This is already a definition list. **Not applicable.**

The documentation at lines 109-124 uses a definition list for the statistics fields. **Correct.**

---

## INFO

### Patch 1/3: Pre-existing code note

The capture library is entirely new code, so there is no pre-existing code to note.

---

### Patch 3/3: Standalone C program

**Info 1: dpdk-wireshark-extcap privilege model**
The extcap plugin uses a careful privilege drop model, similar to `dumpcap`. Privilege is used only for `connect()` to the telemetry socket, then dropped permanently before any reply is parsed. The drop is verified (lines 151-168) and includes capability clearing on Linux. This is a robust design for a setuid helper.

---

## Final Assessment

The patch series introduces a significant new feature with generally high code quality. The primary concern is **Error 1** (resource leak on thread creation failure), which must be fixed. The other errors are either retracted after inspection or require verification of external API behavior (pcapng fdopen ownership).

---

Review-Result: ERROR