|SUCCESS| fib6: fix signedness in common root build
dpdklab at iol.unh.edu
dpdklab at iol.unh.edu
Thu Oct 22 11:00:23 CEST 2026
Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171826
_AI Code Review_
=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-22
# DPDK Patch Review
## Correctness Issues
**Error: Potential sign extension on line with `get_idx()` call**
The patch correctly identifies that `get_idx()` returns `uint32_t`, but the original code stored it in a signed `int idx`. The fix changes `idx` to `uint32_t`. However, there's still a problem on line 325:
```c
idx = get_idx(ip->a, i);
```
This line is not shown in the patch context, but the fix description states `get_idx()` returns `uint32_t`. The patch correctly changes `idx` to `uint32_t` on line 320. This is correct.
**Error: Missing widening cast in existing code visible in patch**
Line 342 in the new code:
```c
*tbl = get_tbl_p_by_idx(cur_tbl,
(uint64_t)prev_idx * TRIE_TBL8_GRP_NUM_ENT, dp->nh_sz);
```
This is actually **correct** - the patch properly widens `prev_idx` to `uint64_t` before multiplication. This matches the guideline for integer multiply without widening cast.
**Review of added widening cast on line 332:**
```c
write_to_dp(tbl_ptr, ((uint64_t)tbl8_idx << 1) |
TRIE_EXT_ENT, dp->nh_sz, 1);
```
This widening cast is correct and necessary. `tbl8_idx` is now `int` (can be negative for error codes from `tbl8_alloc()`), but when used here it's known to be non-negative. The cast to `uint64_t` before shifting prevents any sign extension issues and ensures the shift doesn't overflow if `tbl8_idx` has bit 31 set (though in practice tbl8 indices shouldn't be that large).
## Style Issues
None identified. The patch follows DPDK coding style:
- Variable declarations at the start of the block
- Appropriate use of `uint32_t` for unsigned values
- Proper widening casts before operations that could overflow
- Consistent indentation and formatting
## Documentation
**Info: Release notes entry recommended but not required**
This is a correctness bug fix for existing code. While the commit message includes `Fixes:` and `Cc: stable at dpdk.org` tags (which are correct), a release notes entry in `doc/guides/rel_notes/` would help users understand this fix addresses a sign extension bug that could produce bogus table pointers. However, per the guidelines, release notes are not strictly required for bug fixes, only for API changes and new features.
## Summary
The patch correctly addresses a sign extension bug where `get_idx()` returned `uint32_t` but was stored in `int`, causing negative values when bit 31 was set. The fix:
1. Makes `idx` and `prev_idx` unsigned (`uint32_t`)
2. Introduces a separate signed `tbl8_idx` to handle `tbl8_alloc()` error returns
3. Properly widens to `uint64_t` before shift and multiply operations
The implementation is correct and follows DPDK coding standards.
Review-Result: CLEAN
More information about the test-report
mailing list