|WARNING| [v3] net/af_packet: add option to ignore outgoing packets

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/af_packet ignore_outgoing option

## Summary

This patch adds a new devarg `ignore_outgoing=<0|1>` to the AF_PACKET PMD to enable `PACKET_IGNORE_OUTGOING`, preventing the socket from receiving packets transmitted by the host on the same interface.

## Errors

None identified.

## Warnings

### 1. Error path cleanup incomplete in test function

**File:** `app/test/test_pmd_af_packet.c:967-975`

In `test_af_packet_ignore_outgoing()`, the error path cleanup could be improved.
If port configuration fails, the function sets `err` and jumps to `fail_setup`,
but the allocated mbufs in the `bufs` array are not freed if `alloc_tx_mbufs()` succeeds.

```c
allocated = alloc_tx_mbufs(bufs, 4);
nb_tx = do_tx_burst(tx_port, 0, bufs, allocated);
if (allocated == 0 || nb_tx == 0) {
    err = "TX setup failed";
    goto fail_setup;  /* mbufs not freed here */
}
```

Suggested fix: Add mbuf cleanup on error paths:

```c
allocated = alloc_tx_mbufs(bufs, 4);
nb_tx = do_tx_burst(tx_port, 0, bufs, allocated);
if (allocated == 0 || nb_tx == 0) {
    err = "TX setup failed";
    if (allocated > 0)
        rte_pktmbuf_free_bulk(bufs, allocated);
    goto fail_setup;
}
```

### 2. Missing explicit error check on port configuration

**File:** `app/test/test_pmd_af_packet.c:960-962`

The combined `if` statement checking three `configure_af_packet_port()` calls does not identify which specific port failed.
While functionally correct, separate checks would provide better diagnostics.

Current:
```c
if (configure_af_packet_port(rx_port_ign_out_on, 1, 1) != 0 ||
    configure_af_packet_port(rx_port_ign_out_off, 1, 1) != 0 ||
    configure_af_packet_port(tx_port, 1, 1) != 0) {
    err = "Failed to configure ports";
    goto fail_setup;
}
```

Suggested improvement (optional, for better diagnostics):
```c
if (configure_af_packet_port(rx_port_ign_out_on, 1, 1) != 0) {
    err = "Failed to configure ignore_outgoing=1 port";
    goto fail_setup;
}
if (configure_af_packet_port(rx_port_ign_out_off, 1, 1) != 0) {
    err = "Failed to configure ignore_outgoing=0 port";
    goto fail_setup;
}
if (configure_af_packet_port(tx_port, 1, 1) != 0) {
    err = "Failed to configure TX port";
    goto fail_setup;
}
```

### 3. Incomplete cleanup in test error paths

**File:** `app/test/test_pmd_af_packet.c:986-1003`

The test function's goto-based cleanup jumps incorrectly handle partial initialization:

- If `create_af_packet_port()` for `rx_port_ign_out_on` fails, the function jumps to `fail_rx_ign_on` which calls `TEST_ASSERT(err == NULL, ...)` immediately -- but that assertion will fail and report the error string, which is correct behavior.
  
- However, if port creation succeeds but configuration or the test body fails, the cleanup path calls `rte_eth_dev_stop()` and `rte_eth_dev_close()` even if the device was never started.
  The pattern assumes `configure_af_packet_port()` starts the device, which should be verified.

Recommendation: Verify that calling `rte_eth_dev_stop()` on a configured-but-not-started device is safe, or track start state separately.

### 4. Race condition in RX polling loop

**File:** `app/test/test_pmd_af_packet.c:976-984`

The test polls both RX ports in a loop until `rx_off >= nb_tx` or timeout.
However, it continues polling `rx_port_ign_out_on` even after the loop exit condition is met.
If packets arrive on `rx_port_ign_out_on` very late (edge case), they could be counted after the test has logically "completed."

This is a minor test robustness issue, not a correctness bug, but the loop could be clearer:

```c
while (elapsed < LOOPBACK_TIMEOUT_US && rx_off < nb_tx) {
    rx_off += do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE);
    rx_on += do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE);
    rte_delay_us_block(STATS_POLL_INTERVAL_US);
    elapsed += STATS_POLL_INTERVAL_US;
}
```

## Info

### 1. `parse_uint()` max value check

**File:** `drivers/net/af_packet/rte_eth_af_packet.c:1290-1292`

The `parse_uint(..., &ignore_outgoing, 1)` call limits the value to 1 (boolean 0 or 1).
The test correctly verifies that `ignore_outgoing=2` is rejected.
Good validation.

### 2. Error message formatting

**File:** `drivers/net/af_packet/rte_eth_af_packet.c:1034-1036`

The error message is clear and follows DPDK conventions. No issue.

### 3. Documentation completeness

The documentation in `doc/guides/nics/af_packet.rst` and release notes correctly describes the new parameter. No gaps identified.

### 4. Test coverage

The test `test_af_packet_ignore_outgoing()` verifies:
- Packets are received when `ignore_outgoing=0`
- Packets are NOT received when `ignore_outgoing=1`
- Invalid value `ignore_outgoing=2` is rejected

This provides good functional coverage of the new feature.

---

## Conclusion

The patch is largely correct and well-implemented. The primary concern is the minor resource leak potential in the test's error path (allocated mbufs not freed). The other warnings are code quality suggestions that would improve diagnostics and robustness but do not represent correctness bugs.