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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: series.patch

## Summary
This patch fixes naming conventions for RIB and FIB memory objects and adds length validation. The changes are generally correct, but there are several issues to address.

---

## Errors

### 1. Incorrect sizeof usage for string length calculation (rte_rib.c, rte_rib6.c)

**Location:** `lib/rib/rte_rib.c:433` and `lib/rib/rte_rib6.c:497`

**Issue:**
```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 of the string literal. For a user-facing character count limit, you want `strlen(RIB_MEM_PREFIX)` or `(sizeof(RIB_MEM_PREFIX) - 1)`.

**Why it matters:** The error message will report the limit as one character shorter than the actual allowed length, confusing users.

**Suggested fix:**
```c
RIB_LOG(ERR, "RIB name %s is too long, limit is %zu characters",
    name, RTE_MEMPOOL_NAMESIZE - (sizeof(RIB_MEM_PREFIX) - 1));
```

Same issue in `rte_rib6.c:497`.

---

### 2. Missing documentation updates for test function name truncation

**Location:** `app/test/test_fib6.c`

**Issue:** Several test function names were truncated (e.g., `test_mult_crt`, `test_adddel_inv`, `test_inval_rcu`, etc.) but there's no comment explaining why or documenting the original names for future reference.

**Why it matters:** Makes code maintenance harder; reviewers and future maintainers won't know the rationale.

**Suggested fix:** Add a comment block at the top of the file explaining the name length constraints:
```c
/*
 * Note: RIB/FIB test names are limited to stay within the derived
 * mempool name length constraints after FIB6_MEM_PREFIX is added.
 * Maximum FIB6 name length is 15 characters.
 */
```

---

## Warnings

### 1. Release notes formatting inconsistency

**Location:** `doc/guides/rel_notes/release_26_11.rst:104`

**Issue:** Missing space after comma in the sentence:
```
As the prefixes above are added on top of the name,the new maximum length of
```

Should be:
```
As the prefixes above are added on top of the name, the new maximum length of
```

---

### 2. Release notes list formatting

**Location:** `doc/guides/rel_notes/release_26_11.rst:105-108`

**Issue:** The list uses inconsistent punctuation (mix of periods and no periods) and could use a definition list structure for better clarity.

**Current:**
```
  RIB  - 21 characters.
  RIB6 - 20 characters.
  FIB  - 17 characters.
  FIB6 - 15 characters.
```

**Suggested (definition list for better structure):**
```
RIB
   21 characters maximum.

RIB6
   20 characters maximum.

FIB
   17 characters maximum.

FIB6
   15 characters maximum.
```

Or keep the compact format but make punctuation consistent (either all have periods or none do).

---

### 3. Inconsistent use of `unlikely()` in new validation code

**Location:** `lib/rib/rte_rib.c:432` and `lib/rib/rte_rib6.c:495`

**Issue:** The new `snprintf` length check uses `unlikely()`, but the immediately following `rte_mempool_create()` NULL check does not. Since both are error paths, be consistent.

**Current in rte_rib.c:**
```c
if (unlikely(ret < 0 || ret >= (int)RTE_MEMPOOL_NAMESIZE)) {
    /* ... */
}

node_pool = rte_mempool_create(mem_name, conf->max_nodes, ...);

if (node_pool == NULL) {  /* No unlikely() */
    /* ... */
}
```

**Suggested:** Either remove `unlikely()` from the new check (simpler, as this is not a hot path) or add it to the existing NULL check for consistency. Removing is preferred since this is a create function, not a fast path.

---

### 4. Test name truncation loses readability

**Location:** `app/test/test_fib6.c` (multiple occurrences)

**Issue:** Some truncated names are cryptic:
- `test_mult_crt` (was presumably `test_mult_create` or `test_multiple_create`)
- `test_adddel_inv` (was presumably `test_add_del_invalid`)
- `test_inval_rcu` (was presumably `test_invalid_rcu`)
- `test_drift_comp` (was presumably `test_drift_compression`)
- `test_drift_str` (was presumably `test_drift_stress`)
- `test_drift_tp` (was presumably `test_drift_tight_pool`)

Better truncations would be:
- `test_multi_crt` or `test_mult_create`
- `test_adddel_inv` - `test_adddel_inval`
- `test_inval_rcu` - `test_invalid_rcu` (already 15 chars, might fit depending on exact calculation)
- `test_drift_comp` - acceptable
- `test_drift_str` - `test_drift_stres` (keeps more letters)
- `test_drift_tp` - `test_drift_tight` (clearer)

---

## Info

### 1. Validate the character limits match the documentation

The release notes state:
```
RIB  - 21 characters.
RIB6 - 20 characters.
FIB  - 17 characters.
FIB6 - 15 characters.
```

Verify this matches the actual constraints:
- `RTE_MEMPOOL_NAMESIZE` is typically 32
- `RIB_MEM_PREFIX` is "RIB_" (4 chars + 1 null = 5 bytes)
- Max RIB name = 32 - 4 - 1 = 27 chars (not 21)

Unless there's a secondary constraint from `RTE_RIB_NAMESIZE` (64), the documented limits seem overly conservative. Double-check the calculation and ensure the documented limits match the actual validation code.

---

### 2. Consider adding a compile-time assert

**Location:** `lib/rib/rte_rib.c`, `lib/rib/rte_rib6.c`

**Suggestion:** Add static assertions to catch if `RTE_MEMPOOL_NAMESIZE` ever becomes smaller than the prefix length plus a reasonable minimum name:

```c
RTE_BUILD_BUG_ON(RTE_MEMPOOL_NAMESIZE < sizeof(RIB_MEM_PREFIX) + 10);
```

This prevents silent truncation if constants change in the future.

---

## Correctness Verification

**No correctness bugs found.** The patch does not introduce resource leaks, use-after-free, race conditions, or other memory safety issues. Error paths properly set `rte_errno` and return NULL. The name length validation prevents buffer overflows in `snprintf`.

---

## Style Compliance

**Compliant** with DPDK C coding style. No forbidden tokens, proper indentation, appropriate use of `snprintf`, and correct error handling patterns.

---

## Final Recommendation

**Conditional Accept** -- Fix the `sizeof` vs string length issue (Error #1), add documentation about test name constraints (Error #2), and fix the release notes typo (Warning #1). The other warnings are minor and can be addressed at the maintainer's discretion.