|WARNING| [v3] 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/166927

_AI Code Review_

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

# DPDK Patch Review

## Patch: Add buffer size parameter to rte_eth_dev_get_name_by_port()

---

## Errors

**None found.**

---

## Warnings

**1. Missing volatile-to-atomic conversion (new code, not touched by patch)**

The patch modifies `app/dumpcap/main.c` which uses:
```c
while (!rte_atomic_load_explicit(&quit_signal, rte_memory_order_relaxed))
```
This is correct atomic usage for a shared flag. No issue here.

However, I cannot verify from this patch alone whether the coccinelle scripts have already converted all `volatile` shared variables to atomics throughout the codebase, as that context is not visible.

**2. Function versioning removed without migration plan**

The commit message states "drop function versioning" in v3. The patch adds `use_function_versioning = true` to meson.build but does not include any symbol versioning macros in rte_ethdev.c. This is an ABI-breaking change to a stable function signature.

The release notes document the ABI change, but on a non-LTS release (26.11), ABI breaks are allowed. However, the guidelines suggest using versioning when changing stable function signatures. The patch should either:
- Include versioning macros (`RTE_VERSION_SYMBOL`, `RTE_DEFAULT_SYMBOL`) to maintain backward compatibility, OR
- Document why versioning was intentionally omitted (e.g., if the next LTS is far enough away)

The current approach (adding `use_function_versioning = true` without using versioning macros) is confusing.

**3. Release notes location**

The patch updates `doc/guides/rel_notes/release_26_11.rst`. Verify this is the correct current release file for the target branch. (This is assumed correct based on patch metadata showing July 2026 date.)

---

## Info

**1. Correctness improvements in this patch**

The patch fixes several real correctness issues:
- Eliminates buffer overflow risk from unbounded `strcpy()`
- Adds bounds checking with `strlcpy()` and returns `-ERANGE` on truncation
- Moves the copy operation inside the lock, preventing TOCTOU race where another process could mutate `eth_dev_shared_data->data[port_id].name` between the lock release and the `strcpy()`
- All in-tree callers correctly updated to pass `sizeof(name)`

These are valuable improvements.

**2. Error handling in callers**

Several updated callers now check the return value where they previously did not:
```c
/* pdump cleanup - good pattern */
if (rte_eth_dev_get_name_by_port(pt->rx_vdev_id, name, sizeof(name)) == 0)
    rte_eal_hotplug_remove("vdev", name);
```

This prevents attempting to remove a vdev with a truncated or error name. The pattern is applied consistently in `app/pdump/main.c`, `app/test/test_event_eth_tx_adapter.c`, and `app/test/test_pmd_ring_perf.c`.

**3. Consistent error propagation**

Callers that already had error checks retain them:
```c
/* dumpcap - error propagation maintained */
if (rte_eth_dev_get_name_by_port(intf->port, intf->name, sizeof(intf->name)) < 0)
    rte_exit(EXIT_FAILURE, "Invalid port number %u\n", intf->port);
```

**4. API documentation clarity**

The updated Doxygen is clear:
- Documents the size parameter
- Specifies minimum required size (`RTE_ETH_NAME_MAX_LEN`)
- Documents new error code (`-ERANGE`)
- Returns are well-defined

**5. Locking improvement noted in code comment**

The new comment explains why the shared data name is used instead of the per-process pointer:
```c
/*
 * Use the shared data name rather than rte_eth_devices[].data->name
 * because VDEV PMDs may overwrite the per-process data pointer.
 */
```

This is helpful for future maintainers.

---

## Summary

The patch makes meaningful correctness improvements by eliminating buffer overflow risk and fixing a TOCTOU race. All in-tree callers are updated correctly. The main concern is the incomplete handling of function versioning (Warning #2 above). Otherwise, the implementation is sound.

**Recommendation:** Clarify the function versioning approach before merging. If versioning is intentionally omitted for 26.11, document why in the commit message or release notes. If versioning is intended, add the macros.