|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

This patch adds a buffer size parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows. Overall the changes are correct and improve API safety. A few minor issues need attention.

---

## Errors

### 1. Missing `sizeof(name)` in pdump_validate_port call (lib/pdump/rte_pdump.c)

**Line 739:**
```c
ret = rte_eth_dev_get_name_by_port(port, name, sizeof(name));
```

The `name` parameter is a function argument (`char *name`), not a local array. `sizeof(name)` evaluates to the size of the pointer (typically 8 bytes), not the buffer size. This will cause `-ERANGE` errors even when the buffer is large enough.

**Reason:** The function signature is `static int pdump_validate_port(uint16_t port, char *name)` where `name` is a pointer. Looking at the call sites (not shown in this patch), the caller must be passing a buffer of known size. Without seeing the caller, we cannot determine the correct size to pass here.

**Fix:** Review the call sites of `pdump_validate_port()` to determine the actual buffer size, then either:
- Add a `size` parameter to `pdump_validate_port()` and pass it through, OR
- If the buffer is always `RTE_ETH_NAME_MAX_LEN`, document this as a requirement and pass `RTE_ETH_NAME_MAX_LEN` explicitly

---

## Warnings

### 1. Missing null-termination guarantee documentation (lib/ethdev/rte_ethdev.c)

**Line 747:** The implementation uses `strlcpy()` which always null-terminates even on truncation. However, the Doxygen comment does not explicitly document this guarantee.

**Suggestion:** Add to the Doxygen comment in `rte_ethdev.h`:
```c
 * @param name
 *   Buffer to store the name. The name will always be null-terminated
 *   even if truncation occurs (when buffer is too small).
```

This is a caller-facing guarantee that should be explicit.

---

### 2. Release notes do not mention ABI break (doc/guides/rel_notes/release_26_11.rst)

The patch adds a parameter to a public API function, which is an ABI break. The release notes mention the API change under "API Changes" but there is no corresponding entry under "ABI Changes".

**Fix:** Add an entry to the "ABI Changes" section:
```rst
* ethdev: ``rte_eth_dev_get_name_by_port()`` signature changed,
  added ``size`` parameter. This is an ABI break requiring
  recompilation of applications.
```

---

### 3. Trace call should happen before error return (lib/ethdev/rte_ethdev.c)

**Line 757:** The trace call `rte_ethdev_trace_get_name_by_port(port_id, name)` happens after truncation is detected and an error is returned. This means the trace will show a truncated name on `-ERANGE` errors.

**Suggestion:** Move the trace call before the truncation check, or only call it on success:
```c
	if (n >= size) {
		RTE_ETHDEV_LOG_LINE(ERR,
				    "ethdev port %u name exceeds buffer size %zu",
				    port_id, size);
		return -ERANGE;
	}

	rte_ethdev_trace_get_name_by_port(port_id, name);
	return 0;
```

This ensures the trace reflects what was actually returned to the caller.

---

## Info

### Positive Observations

- All in-tree callers correctly updated to pass `sizeof(name)`
- Error handling is appropriate (`-ENODEV`, `-EINVAL`, `-ERANGE`)
- Lock is correctly held during the copy operation, fixing the TOCTOU race mentioned in the commit message
- Use of `strlcpy()` is correct for bounds-checked string copy
- The shared data name is used instead of the process-local pointer, which is correct per the comment
- Release notes API Changes section updated
- Code follows DPDK style (explicit NULL checks, explicit zero checks)

### Minor Style Observations (no changes needed)

- The error log messages are clear and actionable
- Function maintains backward compatibility in behavior (still copies the name, just safely)
- The new parameter placement (at the end) is conventional for adding a size to existing APIs