|WARNING| fib6: fix error code propagation on next hop update

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Fri Jun 5 19:43:40 CEST 2026


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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-05

# DPDK Patch Review

## Patch Summary
The patch fixes error code propagation in `lib/fib/trie.c`. When `modify_dp()` fails (e.g., rejecting an out-of-range next hop with `-EINVAL`), the error was silently swallowed and success (0) was incorrectly returned to the caller.

---

## Review Findings

### Correctness Issues

**Error: Missing error check on `rte_rib6_set_nh()` call**

The code updates the RIB node (`rte_rib6_set_nh(node, next_hop)`) only when `modify_dp()` succeeds (`ret == 0`), but does not check whether `rte_rib6_set_nh()` itself succeeded. If `rte_rib6_set_nh()` can fail, the dataplane and RIB state may diverge.

**Suggested fix:**
```c
ret = modify_dp(dp, rib, &ip_masked, depth, next_hop);
if (ret == 0) {
    ret = rte_rib6_set_nh(node, next_hop);
    if (ret != 0) {
        /* Rollback modify_dp if needed, or propagate error */
    }
}
return ret;
```

Verify whether `rte_rib6_set_nh()` can fail. If it cannot, this is acceptable as-is. If it can fail but rollback is not possible, at minimum document the potential for inconsistency.

---

### Code Quality

**Info: Consider adding a comment explaining the fix**

The patch correctly propagates the error from `modify_dp()`, but the logic flow (conditional `set_nh` followed by unconditional return) is slightly non-obvious. A brief comment would improve maintainability:

```c
/* Update dataplane */
ret = modify_dp(dp, rib, &ip_masked, depth, next_hop);
if (ret == 0)
    rte_rib6_set_nh(node, next_hop);  /* Only update RIB on success */

/* Propagate modify_dp result (success or error) */
return ret;
```

---

## Summary

**Errors:** 1 (unchecked return value from `rte_rib6_set_nh()`)  
**Warnings:** 0  
**Info:** 1 (code clarity suggestion)

The core fix (returning `ret` instead of hardcoded `0`) is correct and addresses a real bug where error codes were silently dropped. The remaining concern is whether `rte_rib6_set_nh()` itself can fail and whether that failure needs to be handled.


More information about the test-report mailing list