|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: rte_eth_dev_get_name_by_port() buffer size parameter

## Summary

This patch adds a buffer size parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows and updates all in-tree callers. The implementation correctly uses `strlcpy()` and performs the copy under lock. Overall, the patch is well-executed with proper error handling and documentation updates.

## Errors

None.

## Warnings

1. **lib/ethdev/rte_ethdev.c:749** - Potential correctness issue with variable declaration placement:
   ```c
   size_t n = strlcpy(name, eth_dev_shared_data->data[port_id].name, size);
   rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
   
   if (n >= size) {
   ```
   The `strlcpy()` return value check happens after releasing the lock. While `n` contains the length of the source string (which is immutable once read under lock), this pattern is correct. However, declaring `n` inside the locked region would make the data flow clearer:
   ```c
   size_t n;
   rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
   n = strlcpy(name, eth_dev_shared_data->data[port_id].name, size);
   rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
   ```
   This is a minor stylistic concern, not a functional bug.

2. **examples/multi_process/hotplug_mp/commands.c:37-38** - Variable declaration moved inside loop:
   ```c
   RTE_ETH_FOREACH_DEV(port_id) {
       char dev_name[RTE_DEV_NAME_MAX_LEN] = "";
   ```
   This is acceptable C99 style. The empty string initialization (`= ""`) is unnecessary since `rte_eth_dev_get_name_by_port()` now always writes to the buffer or returns an error. However, it serves as defensive programming in case of future API changes, so this is a minor observation rather than an issue.

3. **app/pdump/main.c:542-543, 549-550** - Error path silently continues:
   ```c
   if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) == 0)
       rte_eal_hotplug_remove("vdev", name);
   ```
   If `rte_eth_dev_get_name_by_port()` fails during cleanup, the vdev is leaked (not removed). This is existing behavior from the original code (where failure to get the name would have left `name` uninitialized), but now it's explicit. Consider logging a warning on failure:
   ```c
   if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) == 0)
       rte_eal_hotplug_remove("vdev", name);
   else
       RTE_LOG(WARNING, PDUMP, "Failed to get name for vdev port %u during cleanup\n",
               pt->rx_vdev_id);
   ```

## Info

1. **Consistency in error checking** - The patch consistently checks return values across all callers. In `app/test-pmd/config.c`, when `rte_eth_dev_get_name_by_port()` fails, the code gracefully skips printing the device name, which is appropriate for display functions.

2. **Lock coverage improvement** - The original code read `tmp` under lock but then called `strcpy()` outside the lock, creating a TOCTOU window. The new implementation performs the entire copy operation under lock, which is correct.

3. **Release notes** - The API change is properly documented in both `release_26_11.rst` and the deprecation notice is correctly removed from `deprecation.rst`.

4. **Doxygen documentation** - The function documentation in `rte_ethdev.h` correctly describes the new parameter and the new return value `-ERANGE`.

## Positive Observations

- All in-tree callers correctly pass `sizeof()` for stack buffers and structure member buffers
- Error handling added where previously missing (e.g., `config.c:1034`)
- The `strlcpy()` usage is correct: returns `strlen(src)`, allowing detection of truncation with `n >= size`
- The zero-size buffer check prevents undefined behavior from `strlcpy(buf, src, 0)`
- The function is properly exported with `RTE_EXPORT_SYMBOL`