|WARNING| [v4] ethdev: add buffer size parameter to rte_eth_dev_get_name_by_port()

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of DPDK Patch: Add buffer size parameter to rte_eth_dev_get_name_by_port()

## Summary

This patch adds a size parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows, replacing `strcpy()` with bounds-checked `strlcpy()`. The implementation moves the string copy under the ethdev shared data lock for race-safety. All in-tree callers are updated.

---

## Errors

None identified.

---

## Warnings

### 1. Missing error handling in cleanup paths

**app/pdump/main.c lines 542-543, 550-551:**

The cleanup function ignores errors from `rte_eth_dev_get_name_by_port()` when removing vdevs, which could leave devices unreleased if the function fails.

```c
/* Current - errors silently ignored */
if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) == 0)
    rte_eal_hotplug_remove("vdev", name);
```

While this may be acceptable in cleanup code where the port could already be gone, it should be verified that all vdevs are reliably removed. Consider logging when the name lookup fails but the port is expected to exist.

### 2. Inconsistent error return value checking

**app/test-pmd/config.c lines 830, 1034:**

Some call sites check `== 0` (success), others check `!= 0` (failure), and line 700 checks `== 0` for success. While not incorrect, standardizing on one pattern improves readability.

Suggest: use `!= 0` consistently for error checks (idiomatic for C APIs returning negative errno).

---

## Info

### 1. Lock scope includes string copy operation

**lib/ethdev/rte_ethdev.c lines 741-746:**

The patch correctly moves the `strlcpy()` call inside the spinlock critical section, preventing TOCTOU races where another process could modify `eth_dev_shared_data->data[port_id].name` between reading and copying. This is a correctness improvement over the original code.

### 2. Consistent buffer sizing

All callers use `sizeof(name)` for stack arrays or `sizeof(field)` for struct members, which is the correct pattern to avoid hardcoding `RTE_ETH_NAME_MAX_LEN`.

### 3. Release notes and deprecation notice properly updated

The deprecation notice is removed and the API change is documented in the 26.11 release notes as required.

---

## Positive Observations

- The patch correctly adds validation for zero-size buffers (line 735-738 in rte_ethdev.c)
- Error messages are clear and include relevant context (port_id, buffer size)
- The `-ERANGE` return value on truncation matches standard C library conventions
- Doxygen updates correctly document the new parameter and return values
- All in-tree callers are updated atomically with the API change
- The tracing call at line 755 is correctly placed after successful copy

---

## Final Verdict

**LGTM with minor recommendations.** The patch is correct and improves API safety. The warnings are suggestions for consistency and potential robustness improvements in caller error handling, not blocking issues.