|WARNING| [v2,2/2] net/nfp: move socket from /tmp to runtime dir

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/2: net/cnxk: move socket from /tmp to runtime dir

### Errors

None.

### Warnings

**Missing release notes** (Warning):
This patch changes the socket path from a fixed `/tmp` location to the EAL runtime directory, which is a user-visible behavior change. Applications or scripts that interact with the eswitch control socket will need to know about the new location. A brief note in the release notes explaining the change would help users.

**Missing error check on `rte_eal_get_runtime_dir()` return** (Warning):
`rte_eal_get_runtime_dir()` can return `NULL` if the runtime directory is not set or initialization failed. The code passes this directly to `snprintf()`, which would cause undefined behavior. Check for `NULL` before using the return value:
```c
const char *runtime_dir = rte_eal_get_runtime_dir();
if (runtime_dir == NULL)
    return -EINVAL;
ret = snprintf(un->sun_path, sizeof(un->sun_path), "%s/%s",
               runtime_dir, CNXK_ESWITCH_CTRL_MSG_SOCK_NAME);
```

**No documentation update for new function** (Warning):
`cnxk_eswitch_ctrl_msg_sock_addr()` is a new internal helper function exposed in the header file. While internal, it would benefit from a brief comment explaining its purpose (builds the control socket path in the EAL runtime directory).

### Info

None.

---

## Patch 2/2: net/nfp: move socket from /tmp to runtime dir

### Errors

None.

### Warnings

**Missing release notes** (Warning):
Similar to patch 1/2, this changes the socket path from `/tmp/<pci>` to the EAL runtime directory. External tools that connect to the CPP bridge socket need to know about the new location. A release note entry would help users adapt.

**Missing error check on `rte_eal_get_runtime_dir()` return** (Warning):
Same issue as patch 1/2. `rte_eal_get_runtime_dir()` can return `NULL`. Check before use:
```c
const char *runtime_dir = rte_eal_get_runtime_dir();
if (runtime_dir == NULL) {
    PMD_CPP_LOG(ERR, "Runtime directory not available. Service failed.");
    return -EINVAL;
}
ret = snprintf(address.sun_path, sizeof(address.sun_path), "%s/nfp_%s",
               runtime_dir, pci_name);
```

**Missing cleanup on early error path** (Warning):
In `nfp_cpp_bridge_service_func()`, if `snprintf()` fails (returns an error indicating the path is too long), the function returns `-EINVAL` without calling `unlink(address.sun_path)` or closing `sockfd`. However, at that point `sockfd` hasn't been created yet, so this is not a resource leak. But if the `snprintf` check is moved after the `socket()` call (which happens in the current code ordering), you'd need to close `sockfd` on the early return. The current code order is fine.

**Inconsistent error message** (Info):
The error message "Socket path too long. Service failed." uses a period after "long" but the other error messages in the function use parenthetical errno details. Consider using a consistent format:
```c
PMD_CPP_LOG(ERR, "Socket path too long (%d). Service failed.", ret);
```
(This is purely stylistic; the current message is acceptable.)

### Info

None.

---

## Summary

Both patches correctly address the security/isolation concern of using a fixed world-writable `/tmp` path by moving the socket to the EAL runtime directory. The main issues are:

1. **Missing `NULL` check on `rte_eal_get_runtime_dir()`** (Warning in both patches) -- this could cause undefined behavior if the runtime directory is not initialized.
2. **Missing release notes** (Warning in both patches) -- user-visible path change should be documented.
3. **Missing documentation for new function** (Warning in patch 1/2) -- internal helper would benefit from a comment.

No correctness bugs (use-after-free, resource leaks, race conditions, etc.) were found. The code correctly checks for path truncation and handles errors on the socket operations. The change from `struct sockaddr` to `struct sockaddr_un` in patch 2/2 is correct and necessary for the longer runtime directory paths.