|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

# DPDK Patch Review: rte_eth_dev_get_name_by_port() Buffer Size Parameter

## Summary
This patch adds a size parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows. The implementation replaces `strcpy()` with `strlcpy()` and now performs the copy under the ethdev shared data lock. Overall, the patch is sound and addresses a real safety issue.

---

## Errors

**1. Incorrect error return on truncation**

In `lib/ethdev/rte_ethdev.c` line 749:
```c
if (n >= size) {
    RTE_ETHDEV_LOG_LINE(ERR, "ethdev port %u name exceeds buffer size %zu",
                        port_id, size);
    return -ERANGE;
}
```

After `strlcpy()` truncates the string (when `n >= size`), the function returns `-ERANGE` without calling the trace point. All other success paths call `rte_ethdev_trace_get_name_by_port()` at line 755. The trace should be emitted in all cases where a name was copied, even if truncated.

**Suggested fix:**
```c
size_t n = strlcpy(name, eth_dev_shared_data->data[port_id].name, size);
rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());

rte_ethdev_trace_get_name_by_port(port_id, name);

if (n >= size) {
    RTE_ETHDEV_LOG_LINE(ERR, "ethdev port %u name exceeds buffer size %zu",
                        port_id, size);
    return -ERANGE;
}

return 0;
```

---

## Warnings

**1. Missing error handling in app/dumpcap/main.c**

At line 501 (`statistics_loop()` function):
```c
if (rte_eth_dev_get_name_by_port(p, name, sizeof(name)) < 0)
    continue;
```

This silently skips ports where name retrieval fails. Given that this is a statistics loop that previously assumed `rte_eth_dev_get_name_by_port()` could not fail for valid ports, the error check should at minimum log a message. Otherwise, statistics for ports with long names (triggering `-ERANGE`) would be silently dropped.

**Suggested improvement:**
```c
if (rte_eth_dev_get_name_by_port(p, name, sizeof(name)) < 0) {
    fprintf(stderr, "Failed to get name for port %u\n", p);
    continue;
}
```

**2. Missing error handling in examples/multi_process/hotplug_mp/commands.c**

At line 40:
```c
rte_eth_dev_get_name_by_port(port_id, dev_name, sizeof(dev_name));
if (strlen(dev_name) > 0)
    cmdline_printf(cl, "%d\t%s\n", port_id, dev_name);
```

The return value of `rte_eth_dev_get_name_by_port()` is ignored. If the function fails and returns an error, `dev_name` remains an empty string (due to the initialization on line 38), so the subsequent `strlen()` check coincidentally works. However, this masks errors including the new `-ERANGE` case where the name was truncated. Should check the return value explicitly.

**Suggested improvement:**
```c
int ret = rte_eth_dev_get_name_by_port(port_id, dev_name, sizeof(dev_name));
if (ret == 0 || ret == -ERANGE)  /* Accept truncated names for display */
    cmdline_printf(cl, "%d\t%s\n", port_id, dev_name);
else
    cmdline_printf(cl, "%d\t<error>\n", port_id);
```

**3. Inconsistent error handling for `-ERANGE` across callers**

Most callers treat any error from `rte_eth_dev_get_name_by_port()` as fatal (early return or skip). However, a truncated name (returning `-ERANGE`) might be acceptable in some display/logging contexts. The API documentation in `rte_ethdev.h` line 5716 states the result is truncated, but does not clarify whether the buffer still contains a valid null-terminated string.

Given that `strlcpy()` always null-terminates (even when truncating), the truncated name is safe to use. Callers that only need to display the name could accept `-ERANGE` as non-fatal. Consider documenting this explicitly in the function description.

**Suggested documentation addition (lib/ethdev/rte_ethdev.h line 5698-5717):**
```c
 * @return
 *   - (0) if successful.
 *   - (-ENODEV) if *port_id* is invalid.
 *   - (-EINVAL) if *name* is NULL or *size* is 0.
 *   - (-ERANGE) if the buffer is too small for the device name;
 *               the result is truncated but still null-terminated.
 *               Callers may use the truncated name for display purposes.
```

---

## Info

**1. Deprecation notice removal**

The patch correctly removes the deprecation notice from `doc/guides/rel_notes/deprecation.rst` (lines 139-141 deleted). This is appropriate since the change is now implemented.

**2. Comment clarification**

The new comment at line 742-745 in `lib/ethdev/rte_ethdev.c` correctly explains why `eth_dev_shared_data->data[port_id].name` is used instead of `rte_eth_devices[].data->name`. This addresses a subtle race condition risk where VDEV PMDs could overwrite the per-process data pointer between check and use.

**3. Lock scope**

The lock is now held only during the `strlcpy()` operation, then released before the trace call. This is correct -- the trace point should not be called while holding the lock (good design for performance and avoiding potential trace-subsystem deadlocks).

---

## Overall Assessment

The patch correctly addresses the buffer overflow risk and improves the API by adding bounds checking. The implementation is sound, with one error (missing trace on truncation path) and a few warnings about error handling consistency in callers. The new API is more defensive and follows DPDK conventions.