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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-22

# DPDK Patch Review

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

**No issues found.**

This patch correctly fixes a buffer size mismatch by sizing `pool_name` with `RTE_MEMPOOL_NAMESIZE` and using `sizeof(pool_name)` in `snprintf`, matching the pattern already used in the symmetric path.

---

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

### Errors

None.

### Warnings

1. **ABI break without deprecation notice**

   This patch increases `RTE_MEMZONE_NAMESIZE` from 32 to 64, which is a breaking ABI change affecting multiple core structures (`rte_memzone`, `rte_ring`, `rte_mempool`, `rte_stack`). The release notes document the changes, but there is no prior deprecation notice in a previous release.

   DPDK ABI policy requires deprecation notices one release in advance for breaking changes. This change should have been announced in the 26.07 release notes under `doc/guides/rel_notes/deprecation.rst` before being implemented in 26.11.

2. **Release notes: impact on struct rte_stack not verified**

   The release notes state that `struct rte_stack` grew by one cache line, but the patch does not include changes to `lib/stack/rte_stack.h` to verify this claim or optimize field layout. If `rte_stack` contains datapath fields, they may have been pushed into a second cache line.

   Either include the stack header changes to show the field reordering was considered, or verify and document that the stack structure has no datapath performance impact from the growth.

### Info

1. **struct rte_mempool reordering justification**

   The comment states that reordering keeps `local_cache` and `ops_index` in the first cache line (previously in the second). It would be helpful to verify this claim by showing the actual layout (e.g., using `pahole` output) in the commit message, or at minimum noting the cache line size assumption (64 or 128 bytes).

---

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

### Errors

1. **snprintf return value not checked for truncation in rte_fib.c**

   ```c
   ret = snprintf(mem_name, sizeof(mem_name), FIB_MEM_PREFIX "%s", name);
   if (ret < 0 || ret >= (int)sizeof(mem_name)) {
   ```

   The check `ret >= (int)sizeof(mem_name)` correctly detects truncation, but it compares against `sizeof(mem_name)` which is `RTE_RIB_NAMESIZE` (64 bytes). After prepending `"FIB_"` (4 bytes), the actual limit for the input `name` is 59 characters, not 64. However, the error message in the release notes claims the limit is 49 characters.

   **Suggested fix:** The `mem_name` buffer should be sized `RTE_MEMPOOL_NAMESIZE` (the final destination size), and the check should verify against that. Alternatively, document the actual derivation of the 49-character limit and ensure it accounts for:
   - `FIB_MEM_PREFIX` (4 bytes)
   - `RIB_MEM_PREFIX` (4 bytes)  
   - Total prefix: 8 bytes + null terminator = 9 bytes reserved from `RTE_MEMPOOL_NAMESIZE`

   The same issue exists in `rte_fib6.c` where `FIB6_MEM_PREFIX` is 5 bytes and `RIB6_MEM_PREFIX` is 5 bytes (total 10 bytes + null = 11 bytes reserved).

2. **Inconsistent error message format in rte_rib.c and rte_rib6.c**

   ```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)` includes the null terminator, so this computes `RTE_MEMPOOL_NAMESIZE - 5` = 59 for a 64-byte mempool name. However, the actual limit should account for the prefix length *without* the null terminator (4 characters), giving a limit of 60 - 1 (for the final null) = 59. The formula is correct by accident but confusing.

   **Suggested fix:** Use `strlen(RIB_MEM_PREFIX)` or define a constant `RIB_MEM_PREFIX_LEN` to make the intent clear:
   ```c
   #define RIB_MEM_PREFIX_LEN 4
   RIB_LOG(ERR, "RIB name %s is too long, limit is %d characters",
       name, RTE_MEMPOOL_NAMESIZE - RIB_MEM_PREFIX_LEN - 1);
   ```

   The same issue exists in `rte_rib6.c`.

### Warnings

1. **Release notes: character limits do not match code**

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

   These limits are not clearly derived in the code. The `snprintf` checks guard against truncation but do not enforce these specific limits. Either:
   - Add compile-time assertions (e.g., `RTE_BUILD_BUG_ON`) to verify the arithmetic, or  
   - Show the calculation in comments:
     ```
     FIB limit = RTE_MEMPOOL_NAMESIZE - strlen("FIB_") - strlen("RIB_") - 1
               = 64 - 4 - 4 - 1 = 55 (not 49)
     ```

   The documented limits appear to assume the old 32-byte `RTE_MEMPOOL_NAMESIZE`. With the new 64-byte size from patch 2/3, the limits should be recalculated.

2. **Missing const on string literal defines**

   ```c
   #define FIB_MEM_PREFIX     "FIB_"
   #define FIB6_MEM_PREFIX    "FIB6_"
   ```

   These are used in `snprintf` format strings. While harmless, defining them as `const char[]` instead of bare string literals would be more explicit and allow `sizeof()` to return the correct length (including null terminator) without relying on the preprocessor.

   Not critical, but cleaner style:
   ```c
   static const char FIB_MEM_PREFIX[] = "FIB_";
   ```

---

## Summary

**Patch 1/3:** Clean fix, no issues.

**Patch 2/3:** ABI break requires prior deprecation notice (missing). Release notes claim about `rte_stack` growth not verified in code.

**Patch 3/3:** 
- Error: `snprintf` truncation checks compare against wrong size; name length limits in release notes do not match new 64-byte mempool names.
- Warning: `sizeof(prefix)` used instead of `strlen(prefix)` in error messages is confusing.