|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/167207

_AI Code Review_

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

# DPDK Patch Review

## Summary

The patch adds a `size` parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows. The changes include proper bounds checking, lock-protected copying, and updates to all in-tree callers.

**Overall assessment:** The patch is well-structured and addresses a real safety issue. There are a few minor issues to address.

---

## Errors

### 1. Missing error check propagation in pdump cleanup

**File:** `app/pdump/main.c`

**Issue:** The error return value from `rte_eth_dev_get_name_by_port()` is checked to decide whether to call `rte_eal_hotplug_remove()`, but the actual error (including `-ERANGE` if the buffer is too small) is silently ignored. If the buffer is too small, the device will not be removed, leaking the vdev.

```c
if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) == 0)
    rte_eal_hotplug_remove("vdev", name);
```

**Why it matters:** A `-ERANGE` error means the buffer was too small and the name was truncated. Passing a truncated name to `rte_eal_hotplug_remove()` would fail to remove the device, but the current code silently skips removal. This leaks the vdev resource.

**Suggested fix:** Either handle `-ERANGE` as a fatal error in cleanup (assert/abort), or use `RTE_ETH_NAME_MAX_LEN`-sized buffers throughout to guarantee success.

```c
/* Option 1: assert success in cleanup */
int ret = rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name));
assert(ret == 0);  /* cleanup must succeed */
rte_eal_hotplug_remove("vdev", name);

/* Option 2: log and continue (risky -- leaks vdev) */
if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) < 0) {
    /* Log error but cannot remove device */
    continue;
}
rte_eal_hotplug_remove("vdev", name);
```

### 2. Same issue in test_event_eth_tx_adapter.c deinit

**File:** `app/test/test_event_eth_tx_adapter.c`

```c
if (rte_eth_dev_get_name_by_port(default_params.port[i], name, sizeof(name)) == 0)
    rte_vdev_uninit(name);
```

The same resource leak pattern: if `-ERANGE` is returned, the vdev is not uninitialized. In test cleanup, this is less critical but still incorrect.

**Suggested fix:** Same as above -- either assert success or log the failure explicitly.

---

## Warnings

### 1. Inconsistent error handling in test code

**File:** `app/test/test_pmd_ring_perf.c`

The new pattern `if (rte_eth_dev_get_name_by_port(...) == 0) rte_vdev_uninit(name);` silently skips cleanup on error. While this avoids passing a truncated name to `rte_vdev_uninit()`, it leaks the vdev.

**Suggested approach:** In test cleanup code, where the buffer size is known to be `RTE_ETH_NAME_MAX_LEN`, document that the check is defensive. If the buffer is correctly sized, the function cannot return `-ERANGE` (it can only fail with `-ENODEV` for invalid port IDs, which should not occur in cleanup of previously created devices).

### 2. pdump_validate_port() error message could be clearer

**File:** `lib/pdump/rte_pdump.c`

After the change:

```c
ret = rte_eth_dev_get_name_by_port(port, name, sizeof(name));
if (ret < 0) {
    PDUMP_LOG_LINE(ERR, "port %u to name mapping failed",
              port);
```

The error message does not indicate whether the failure was due to invalid port ID (`-ENODEV`), NULL pointer (`-EINVAL`), or buffer overflow (`-ERANGE`). Since the function now has three distinct error cases, the log message should ideally reflect which one occurred.

**Suggested fix:**

```c
ret = rte_eth_dev_get_name_by_port(port, name, sizeof(name));
if (ret < 0) {
    PDUMP_LOG_LINE(ERR, "port %u to name mapping failed: %s",
                  port, rte_strerror(-ret));
    return -1;
}
```

---

## Info

### 1. Release notes entry location

The release notes update is in `release_26_11.rst`. This is correct for a patch targeting the 26.11 release. If this patch is intended for backport to an LTS branch (e.g., 23.11, 24.11), the release notes entry would need to be in the appropriate LTS release notes file instead.

### 2. Documentation example clarity

The function documentation now states "Should be at least RTE_ETH_NAME_MAX_LEN bytes." This is clear. Consider adding a brief example in the commit message or a code comment showing the typical usage pattern:

```c
char name[RTE_ETH_NAME_MAX_LEN];
if (rte_eth_dev_get_name_by_port(port_id, name, sizeof(name)) < 0)
    /* handle error */
```

This is already implied by the in-tree usage, so not strictly necessary.

### 3. Function versioning not needed

Note: This is an ABI-breaking change (function signature changed). The patch does not use `RTE_VERSION_SYMBOL` / `RTE_DEFAULT_SYMBOL` for ABI versioning. This is acceptable if the target is the next major release (26.11) where ABI breaks are allowed. If backporting to an LTS, ABI versioning would be required.

---

## Compliance Summary

- **Correctness bugs:** 2 errors (resource leak on `-ERANGE` in cleanup paths)
- **Code style:** Compliant
- **API requirements:** 
  - Release notes: present 
  - Doxygen updated: yes 
  - All callers updated: yes 
- **Testing:** No new tests added. Consider adding a unit test in `app/test/test_ethdev.c` that verifies `-ERANGE` is returned for a too-small buffer.

---

## Recommendations

1. **Fix the cleanup error handling** in `app/pdump/main.c` and `app/test/test_event_eth_tx_adapter.c` to either assert success or explicitly handle `-ERANGE`.
2. **Improve error logging** in `lib/pdump/rte_pdump.c` to distinguish the three error cases.
3. **Add a unit test** demonstrating that a buffer smaller than `RTE_ETH_NAME_MAX_LEN` correctly returns `-ERANGE`.