|WARNING| net/af_xdp: fix default device plugin path

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch Summary
This patch fixes the default device plugin path for the AF_XDP PMD to use the EAL runtime directory instead of hardcoded `/tmp/afxdp_dp`, with fallback to the legacy location for backward compatibility.

---

## ERRORS

### 1. Missing error check on `access()` errno
**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Line:** 2492 (`if (access(dp_path, F_OK) == 0)`)

The code checks if `access()` returns 0 (file exists) but does not verify that a non-zero return is specifically `ENOENT` (file does not exist). Other errors like `EACCES` (permission denied) or `ENAMETOOLONG` would cause fallback to the legacy path even though the runtime directory entry exists but is inaccessible.

**Fix:** Check errno explicitly:
```c
if (access(dp_path, F_OK) == 0)
    return 0;

if (errno != ENOENT) {
    AF_XDP_LOG_LINE(ERR, "Cannot access '%s': %s", dp_path, strerror(errno));
    return -errno;
}
```

### 2. Path length check duplicated after construction
**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Lines:** 2625-2630

The `snprintf()` calls in `get_dflt_dp_path()` already check for truncation (`ret >= size`), so the subsequent check at lines 2625-2630 for `use_cni` is redundant. However, this check uses a *different* size (`sizeof(sockaddr_un.sun_path)` vs `PATH_MAX`), so it's not completely redundant. The real issue is that `get_dflt_dp_path()` should enforce the correct limit upfront.

**Fix:** Make `get_dflt_dp_path()` take the actual required size as a parameter, or add a compile-time assertion that `PATH_MAX >= sizeof(sun_path)`. As written, the code could build a path that passes `get_dflt_dp_path()` but then fails at line 2626.

Actually, on further inspection: `dp_path` is declared with size `PATH_MAX`, but the check at 2626 compares against `sizeof(sockaddr_un.sun_path)` which is typically 108 bytes. This is correct -- `snprintf()` into a `PATH_MAX` buffer won't catch the socket-specific limit. However, the check should be applied to *both* the runtime and legacy paths, not just when `use_cni` is set. The pinned map case (line 2608) builds a path but never validates it against socket limits if `use_pinned_map` is true without `use_cni`.

**Actually:** On re-reading, the pinned map doesn't use a socket, so the `sockaddr_un` limit doesn't apply there. The current code is correct for `use_cni` but could be clearer.

**Suggested improvement (Warning level):** Move the socket path length check into `init_uds_sock()` where it now appears (line 1443-1446), which is good. The existing code at 2625-2630 is now dead code and should be removed.

Wait, looking again: the check at 1443-1446 is new in this patch and validates `dp_path` length before use. The check at 2625-2630 is *also* in this patch. This means both checks exist, which is redundant but not wrong. Retracting Error #2.

---

## WARNINGS

### 1. Dead code at lines 2625-2630
**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Lines:** 2625-2630

The path length check for `use_cni` is now redundant because:
- `get_dflt_dp_path()` already checks `snprintf()` return against `sizeof(dp_path)` (which is `PATH_MAX`)
- `init_uds_sock()` now checks `strnlen(dp_path, PATH_MAX) >= sizeof(sun_path)` at line 1443

The check at 2625-2630 can never trigger because either `get_dflt_dp_path()` would have failed earlier (lines 2600/2609) or the explicit `dp_path` argument was already validated by `init_uds_sock()` at line 1443.

**Suggested fix:** Remove lines 2625-2630.

---

## STYLE / PROCESS ISSUES

### 1. Inconsistent path buffer size (Info)

The `dp_path` variable is declared with `PATH_MAX` size, but the actual constraint is `sizeof(sockaddr_un.sun_path)` (typically 108). Consider using a buffer sized to the actual constraint, or adding a compile-time check that `PATH_MAX` is sufficient for the constructed paths.

---

## CORRECTNESS VERIFICATION (No Issues)

The following were verified and are **correct**:
- Error paths in `get_dflt_dp_path()`: both `snprintf()` error cases return `-ENAMETOOLONG` without leaking resources
- The `if_name` length check moved to line 2574 ensures the interface name is non-empty before it's used to construct the default path (good ordering)
- The fallback logic (runtime dir - legacy `/tmp`) preserves backward compatibility
- Documentation updates correctly describe the new behavior and the fallback

---

## SUMMARY

**Errors:** 1 (missing `errno` check on `access()`)  
**Warnings:** 1 (dead code at lines 2625-2630)  
**Info:** 1 (buffer sizing note)

The patch is mostly well-structured. The primary concern is the `access()` call not distinguishing between "file doesn't exist" and other errors, which could cause misleading fallback to `/tmp` when the runtime directory entry exists but is inaccessible.