|WARNING| [v3,3/3] fib, rib: fix name prefixes

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171562

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-16

# DPDK Patch Review

## Patch 1/3: app/crypto-perf: fix asymmetric op pool name size

### Info
Pre-existing style inconsistency (not introduced by this patch): the symmetric path uses `sizeof(pool_name)` for the snprintf limit while the asymmetric path (before this patch) used the constant `RTE_MEMPOOL_NAMESIZE`. This patch resolves that inconsistency.

---

## Patch 2/3: eal: increase memzone name size to 64

### Errors

**Missing RTE_EXPORT_SYMBOL for field reorder in public structures:**
The patch reorders fields in `struct rte_mempool` and `struct rte_ring`, both of which are in installed headers and part of the public ABI. While the release notes document the ABI change, the structures themselves are not functions and do not require `RTE_EXPORT_SYMBOL` annotations. However, the ABI change requires version symbol handling if any inline functions or macros access these fields by offset.

**Correction**: Field reordering in a structure is an ABI break but does not require `RTE_EXPORT_SYMBOL` macros (those apply to functions). The release notes correctly document this as an ABI change. No error here after verification.

**Hardcoded 192 bytes in release notes needs verification:**
```
struct rte_mempool stays 192 bytes.
```
This claim should be verified against the actual structure size. On a 64-bit system with 64-byte cache lines:
- Before: name[32] at offset 0, other fields following
- After: name[64] moved to end, datapath fields first

The claim that size stays 192 bytes assumes the structure was already cache-aligned and the larger name fits in existing padding. This is plausible but should be verified by the build system or static assertion.

**Result**: This is a claim the patch makes but cannot verify from the diff alone. If the structure actually grows, the release notes are wrong. Recommend adding a static assertion in the code to verify the size claim.

**Severity downgrade**: This is a documentation accuracy concern, not a correctness bug. **Info** level.

---

### Warnings

**Release notes ordering:**
The release notes place the memzone size increase under "ABI Changes" but the description mixes behavioral explanation with ABI impact. The structure is correct but could be clearer: the ABI impact is the size and layout change; the motivation (longer names) belongs elsewhere.

**Result**: The content is accurate and complete. Ordering within the section is a style preference. **No finding.**

---

## Patch 3/3: fib, rib: fix name prefixes

### Errors

**Incorrect size limit calculation in error messages:**
```c
RIB_LOG(ERR, "RIB name %s is too long, limit is %zu characters",
    name, RTE_MEMPOOL_NAMESIZE - sizeof(RIB_MEM_PREFIX));
```
`sizeof(RIB_MEM_PREFIX)` is 5 (the string `"RIB_"` plus null terminator). The actual limit on the user-provided name is `RTE_MEMPOOL_NAMESIZE - strlen(RIB_MEM_PREFIX) - 1` (the `-1` accounts for the null terminator of the final concatenated string, which `snprintf` will write). Using `sizeof()` overcounts by 1.

The same error appears in all four locations:
- `lib/rib/rte_rib.c` line for RIB
- `lib/rib/rte_rib6.c` line for RIB6
- Similar pattern would appear in FIB/FIB6 if they had this check (but they don't)

**Fix:**
```c
RIB_LOG(ERR, "RIB name %s is too long, limit is %zu characters",
    name, RTE_MEMPOOL_NAMESIZE - strlen(RIB_MEM_PREFIX) - 1);
```
Or define the limit as a constant and use it in both the check and the message.

---

**Missing name length check in FIB/FIB6:**
The FIB and FIB6 code checks `snprintf` return value and returns `ENAMETOOLONG`, but does not log an error message explaining the limit to the user. The RIB/RIB6 code does log a message. For consistency and usability, FIB/FIB6 should also log.

**Recommended addition** in `lib/fib/rte_fib.c` and `lib/fib/rte_fib6.c`:
```c
if (ret < 0 || ret >= (int)sizeof(mem_name)) {
    FIB_LOG(ERR, "FIB name %s is too long, limit is %zu characters",
        name, sizeof(mem_name) - sizeof(FIB_MEM_PREFIX));
    rte_errno = ENAMETOOLONG;
    return NULL;
}
```
And similarly for FIB6 with `FIB6_MEM_PREFIX`.

**Severity**: The missing log is a usability issue, not a correctness bug. **Warning**.

The incorrect size calculation in the existing RIB/RIB6 messages is an **Error** because it gives the user wrong information (off by one).

---

### Warnings

**Release notes character limits need verification:**
The release notes claim:
```
RIB : 53 characters
RIB6: 52 characters
FIB : 49 characters
FIB6: 47 characters
```

Verification (assuming `RTE_MEMPOOL_NAMESIZE` is 64 after patch 2):
- RIB: `RTE_MEMPOOL_NAMESIZE (64) - strlen("RIB_") (4) - 1 (null) = 59`, not 53
- RIB6: `64 - strlen("RIB6_") (5) - 1 = 58`, not 52
- FIB: RIB is derived as `"FIB_" + name`, then RIB is `"RIB_" + ("FIB_" + name)`. Wait, no. Re-reading the code:

FIB creates a RIB with name `"FIB_" + original_name`. That RIB then creates a mempool with name `"RIB_" + ("FIB_" + original_name) = "RIB_FIB_" + original_name`. So the limit on `original_name` is:
```
64 - strlen("RIB_FIB_") (8) - 1 = 55
```
Not 49.

Similarly for FIB6:
```
64 - strlen("RIB6_FIB6_") (10) - 1 = 53
```
Not 47.

The release notes numbers are wrong. They may be computing `RTE_RIB_NAMESIZE - prefix` but that's not the right bound--the constraint is `RTE_MEMPOOL_NAMESIZE` on the final concatenated name.

**Fix**: Recalculate the limits correctly or explain the derivation. The actual limits are:
- RIB: 59 characters
- RIB6: 58 characters  
- FIB: 55 characters
- FIB6: 53 characters

**Severity**: **Error** (incorrect API documentation misleads users).

---

### Info

**Inconsistent error message format:**
RIB/RIB6 use `"RIB name %s is too long"` while FIB/FIB6 use `"FIB name %s is too long"`. This is fine, but RIB/RIB6 include the limit in the message while FIB/FIB6 do not (as noted under Warnings). Consistency would improve usability.

---

## Summary

### Patch 1/3: CLEAN
- Info-level note about pre-existing inconsistency that this patch resolves.

### Patch 2/3: CLEAN (after evidence review)
- Info-level note about structure size claim in release notes (cannot verify from diff, recommend static assertion).

### Patch 3/3: ERROR
- **Error**: `sizeof(RIB_MEM_PREFIX)` used instead of `strlen(RIB_MEM_PREFIX)` in error messages (off-by-one).
- **Error**: Release notes document incorrect character limits (miscalculation of prefix stacking).
- **Warning**: FIB/FIB6 missing user-facing error messages for name length (RIB/RIB6 have them).

---

## Detailed Findings

### lib/rib/rte_rib.c (and rte_rib6.c)

**Error**: Incorrect limit in error message.
```c
RIB_LOG(ERR, "RIB name %s is too long, limit is %zu characters",
    name, RTE_MEMPOOL_NAMESIZE - sizeof(RIB_MEM_PREFIX));
```
`sizeof("RIB_")` is 5 (includes null terminator). The usable space for the user's name is `RTE_MEMPOOL_NAMESIZE - strlen("RIB_") - 1 = 64 - 4 - 1 = 59`, not `64 - 5 = 59`. Wait, that's the same. Let me recalculate.

Actually: `sizeof("RIB_")` = 5. `strlen("RIB_")` = 4. The final buffer is 64 bytes. After writing `"RIB_"` (4 chars) and the user's name, we need 1 byte for null. So max user name length is `64 - 4 - 1 = 59`.

Using `sizeof(RIB_MEM_PREFIX)` gives `64 - 5 = 59`, which is accidentally correct. But using `sizeof()` on a string is the wrong semantic--it's coincidentally right only because the calculation was done backwards. The correct formulation is `strlen(prefix)`, and the `-1` is explicit.

**Revised finding**: The calculation happens to be correct but is semantically wrong. Using `sizeof()` on a string literal to compute name length is a code smell and will break if the prefix ever becomes a const char array instead of a literal.

**Downgrade to Warning**: The value is correct; the method is poor style.

**Wait, rechecking**: 
- `sizeof("RIB_")` = 5 (includes null terminator of the string literal).
- When `snprintf(mem_name, 64, "RIB_%s", name)` runs, it writes `strlen("RIB_") + strlen(name) + 1` bytes (the +1 is the final null terminator).
- For this to fit in 64 bytes: `4 + strlen(name) + 1 <= 64`, so `strlen(name) <= 59`.
- `RTE_MEMPOOL_NAMESIZE - sizeof(RIB_MEM_PREFIX) = 64 - 5 = 59`. This is correct.

So the current code is correct. Using `sizeof()` for a string literal is actually the right approach here because we want the size of the literal including its embedded null terminator (which we're replacing with the concatenation point), not the strlen.

**Final verdict**: The code is correct. **No finding**.

---

### doc/guides/rel_notes/release_26_11.rst

**Error**: Incorrect character limits documented.

The release notes state:
```
* RIB  - 53 characters.
* RIB6 - 52 characters.
* FIB  - 49 characters.
* FIB6 - 47 characters.
```

Let me trace through the actual limits:

1. **RIB**: User provides name `N`. RIB creates mempool `"RIB_" + N`. Limit: `strlen(N) <= 64 - 4 - 1 = 59`.

2. **RIB6**: Mempool is `"RIB6_" + N`. Limit: `strlen(N) <= 64 - 5 - 1 = 58`.

3. **FIB**: User provides name `N`. FIB creates RIB with name `"FIB_" + N`. RIB creates mempool `"RIB_" + ("FIB_" + N) = "RIB_FIB_" + N`. 
   Limit: `strlen("FIB_" + N) <= 59` (the RIB limit from step 1).
   So `strlen(N) <= 59 - 4 = 55`.

4. **FIB6**: RIB6 is named `"FIB6_" + N`, mempool is `"RIB6_" + ("FIB6_" + N) = "RIB6_FIB6_" + N`.
   Limit: `strlen("FIB6_" + N) <= 58` (the RIB6 limit from step 2).
   So `strlen(N) <= 58 - 5 = 53`.

The documented limits (53, 52, 49, 47) are all **wrong**. The correct limits are (59, 58, 55, 53).

This is an **Error** in the release notes.

---

### lib/fib/rte_fib.c and rte_fib6.c

**Warning**: Missing user-facing error message explaining the character limit.

The code checks the length and returns `ENAMETOOLONG`, but does not log a message telling the user what the limit is. The RIB code does log such a message. For consistency:

```c
if (ret < 0 || ret >= (int)sizeof(mem_name)) {
    FIB_LOG(ERR, "FIB name %s is too long, limit is %zu characters",
        name, sizeof(mem_name) - sizeof(FIB_MEM_PREFIX));
    rte_errno = ENAMETOOLONG;
    return NULL;
}
```

---

## Final Summary

**Patch 1/3**: CLEAN

**Patch 2/3**: CLEAN (with Info note on structure size claim)

**Patch 3/3**: 
- **Error**: Release notes document incorrect character limits (should be 59, 58, 55, 53; documented as 53, 52, 49, 47).
- **Warning**: FIB and FIB6 do not log an error message when the name is too long (RIB and RIB6 do).

Review-Result: ERROR