|WARNING| [v3] examples/rpcapd: demo version of packet capture daemon

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-22

# DPDK Patch Review: examples/rpcapd

## Summary
This patch adds a new example application implementing the rpcap protocol for remote packet capture. Overall, the code demonstrates good structure and follows many DPDK conventions. Below are findings organized by severity.

---

## Errors

### 1. Resource leak on error path in `handle_startcap()`
**Location:** `examples/rpcapd/main.c:665`

The `data_listen` socket is not closed if `rte_pdump_enable_bpf()` fails after the socket has been successfully opened.

```c
if (rte_pdump_enable_bpf(...) < 0) {
    RPCAPD_LOG(ERR, "rte_pdump_enable_bpf port %u failed: %s",
        s->port, rte_strerror(rte_errno));
    close(data_listen);  // <-- this is correctly placed
    stop_capture(s);
    return rpcap_send_error(fd, 0, "cannot enable capture");
}
```

Wait, reviewing more carefully - the code DOES close `data_listen` on the error path. However, there's a gap: if `rpcap_send_msg()` fails at line 689, the function returns -1 without closing `data_listen`.

**Fix:**
```c
if (rpcap_send_msg(fd, RPCAP_MSG_STARTCAP_REPLY, 0, &reply, sizeof(reply)) < 0) {
    close(data_listen);  // <-- add this
    stop_capture(s);
    return -1;
}
```

### 2. Resource leak in `dpdk_init()` on allocation failure
**Location:** `examples/rpcapd/main.c:1239-1251`

When `strdup()` calls fail in the EAL argv construction loop, the function returns -1 without freeing the previously allocated strings in `eal_argv`.

**Fix:** Add cleanup on all error paths:
```c
if (eal_argv[i] == NULL) {
    for (unsigned int j = 0; j < i; j++)
        free(eal_argv[j]);
    free(eal_argv);
    return -1;
}
```

### 3. `eal_argv` array never freed (memory leak)
**Location:** `examples/rpcapd/main.c:1230-1258`

After `rte_eal_init()` returns, the `eal_argv` array and all its string elements are never freed. This is a one-time leak at startup, but should be cleaned up.

**Fix:** After `rte_eal_init()` (line 1260), add:
```c
for (unsigned int j = 0; j < eal_argc; j++)
    free(eal_argv[j]);
free(eal_argv);
```

### 4. Integer overflow risk in `handle_updatefilter()`
**Location:** `examples/rpcapd/main.c:575`

When checking filter instruction count, the multiplication `nitems * sizeof(winsn)` is performed on `uint32_t` values, which could overflow before comparison with `plen` (also `uint32_t`). For `MAX_FILTER_INSNS = 4096` this is safe, but the code does not enforce the relationship.

**Fix:** Perform the size check with widening:
```c
if (nitems > MAX_FILTER_INSNS || 
    plen < (uint64_t)nitems * sizeof(winsn)) {
```

---

## Warnings

### 1. Missing release notes for experimental API usage
**Location:** `examples/rpcapd/main.c:38-39`

The example uses `rte_bpf_convert()` which is an experimental API (from `rte_bpf.h`). The release notes mention the example but do not call out that it relies on experimental APIs.

**Suggestion:** Add a note in `doc/guides/rel_notes/release_26_11.rst` that the example uses experimental `rte_bpf_convert()`.

### 2. Queue-related structure allocated with `malloc()` instead of `rte_zmalloc_socket()`
**Location:** `examples/rpcapd/main.c:105-107, 1082-1085`

The `session` structure is stack-allocated in `handle_client()`. This is acceptable for control plane state, but the structure contains pointers to rings and mempools that are shared-memory DPDK objects. The `session` structure itself does not need hugepage backing since it's process-local.

**Assessment:** Not a bug - the session structure is local control state, not a queue descriptor or shared structure. The ring and mempool it points to are correctly allocated with DPDK APIs. No action needed.

### 3. `rte_pktmbuf_free_bulk()` used in error path with potentially mixed pools
**Location:** `examples/rpcapd/main.c:867`

In `process_ring()` error handling, when `send_iov_full()` fails mid-burst, the remaining packets are freed with `rte_pktmbuf_free_bulk()`. Since these mbufs all came from the same capture mempool (`s->mp`), this is safe. However, the code could be clearer about this invariant.

**Assessment:** Actually correct - all packets in the ring came from the same mempool created in `handle_startcap()`. No issue.

### 4. `bool` not used for true/false variables
**Location:** Multiple locations

Several variables that are purely true/false are declared as `int` instead of `bool`:
- `main.c:1334`: `int srv_fd;` should be `int` (it's a file descriptor, not a boolean)

Actually, reviewing the code, most boolean variables ARE correctly declared as `bool` in the `session` structure (lines 87-89). This is good practice.

**Assessment:** Code already follows the guideline. No issue.

### 5. Statistics `svrcapt` uses `=` instead of `+=`
**Location:** `examples/rpcapd/main.c:976`

Wait, reviewing the code:
```c
struct rpcap_stats reply = {
    .ifrecv   = htonl((uint32_t)es.ipackets),
    .ifdrop   = htonl((uint32_t)es.ierrors),
    .krnldrop = 0,
    .svrcapt  = htonl(s->npkt),
};
```

This is an assignment to a local structure being sent in a reply, not accumulation. And `s->npkt` is incremented with `s->npkt++` at line 821. This is correct.

**Assessment:** No issue.

---

## Info / Style

### 1. Explicit NULL comparisons
The code consistently uses explicit comparisons (`if (ptr == NULL)`, `if (fd < 0)`) which follows DPDK style. Good.

### 2. `rte_atomic_*` usage
The `quit_signal` variable correctly uses `rte_atomic_*_explicit()` with appropriate memory ordering (`relaxed` for a simple flag). Good.

### 3. Documentation matches implementation
The limitations section in `rpcapd.rst` accurately describes the single-client limitation, TCP-only transport, and lack of original length reporting. Good.

### 4. Release notes
The release notes entry is present and describes the feature at an appropriate level. Good.

### 5. Maintainer entry
MAINTAINERS file correctly updated with the new files. Good.

### 6. Hardcoded overhead constant
**Location:** `examples/rpcapd/main.c:65`

The code uses `DEFAULT_SNAPLEN = RTE_MBUF_DEFAULT_DATAROOM`, which is a reasonable default and not an Ethernet overhead calculation issue. The snaplen is provided by the client in the STARTCAP request. No issue.

---

## Summary of Required Fixes

### Errors (must fix):
1. Close `data_listen` socket when `rpcap_send_msg()` fails in `handle_startcap()`
2. Free allocated `eal_argv` strings on error in `dpdk_init()`
3. Free `eal_argv` array and strings after successful `rte_eal_init()`
4. Use widening cast in filter size check to prevent overflow

### Warnings (should fix):
1. Mention experimental API dependency in release notes

### Code Quality:
- Overall the code demonstrates good resource management patterns
- Atomics usage is correct
- Documentation is thorough and matches implementation
- Error handling is generally good with the exceptions noted above

The patch represents a substantial, well-structured example application. The resource leaks identified are the primary concerns that must be addressed before merging.