Re: [v3 0/3] L3fwd changes

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 20 Nov 2024 09:35:13 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> v3 changes:
> *  rebased the series to latest commit.
> 
> v2 changes:
> *  Handled a comment to enhance the invalid port ID logic
>    and added a user option to decide exit or silently skip
>    in case invalid port in the rules list.
> 
> Gagandeep Singh (3):
>   examples/l3fwd: support single route file
>   examples/l3fwd: fix return value on rules add
>   examples/l3fwd: enhance valid ports checking
> 
>  examples/l3fwd/em_route_parse.c  | 33 +++++++++---------
>  examples/l3fwd/l3fwd.h           | 16 +++++++++
>  examples/l3fwd/l3fwd_em.c        | 22 ++++++++----
>  examples/l3fwd/l3fwd_fib.c       | 26 +++++++++-----
>  examples/l3fwd/l3fwd_lpm.c       | 26 +++++++++-----
>  examples/l3fwd/l3fwd_route.h     |  2 ++
>  examples/l3fwd/lpm_route_parse.c | 28 ++++++++-------
>  examples/l3fwd/main.c            | 58 +++++++++++++++++++++++++++-----
>  8 files changed, 153 insertions(+), 58 deletions(-)
> 


These patches appear to have gotten stuck in the backlog.
Running automated patch review shows minor issues.
Please rebase, fix issues and resubmit.


## DPDK Patch Review: l3fwd Patchset v3 (bundle-1634)

### Overview
This patchset contains 3 patches by Gagandeep Singh targeting the l3fwd example application.

---

## Patch 1/3: `examples/l3fwd: support single route file`

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ Pass | 41 characters |
| Lowercase after colon | ✅ Pass | |
| Correct prefix | ✅ Pass | `examples/l3fwd:` is correct |
| No trailing period | ✅ Pass | |
| Imperative mood | ✅ Pass | "support" |
| Body wrapped ≤75 chars | ✅ Pass | |
| Body doesn't start with "It" | ✅ Pass | |
| Signed-off-by present | ✅ Pass | Line 108 |

### Code Review

| Check | Status | Notes |
|-------|--------|-------|
| Lines ≤100 chars | ✅ Pass | |
| Consistent style | ✅ Pass | |

**Info:** This is a feature patch that decouples IPv4 and IPv6 rule file requirements. Logic looks sound.

---

## Patch 2/3: `examples/l3fwd: fix return value on rules add`

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ Pass | 45 characters |
| Lowercase after colon | ✅ Pass | |
| Correct prefix | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Body wrapped ≤75 chars | ✅ Pass | |
| Body doesn't start with "It" | ✅ Pass | Starts with "fix" |
| Signed-off-by present | ✅ Pass | |
| Fixes: tag present | ✅ Pass | Two Fixes tags (lines 304-305) |
| Fixes: format (12-char SHA) | ✅ Pass | `e7e6dd643092`, `52def963fc1c` |
| Cc: stable@dpdk.org | ✅ Pass | Line 307 |
| Tag order correct | ✅ Pass | Fixes → Cc → blank → Signed-off-by |

### Issues

**Warning:** The commit body is very terse — just one line:
```
fix return value on adding the EM or LPM rules.
```
Consider expanding to explain *why* this matters (e.g., "The original code always returned -EINVAL, losing the actual parser error code which is needed for debugging").

### Code Review

| Check | Status | Notes |
|-------|--------|-------|
| Lines ≤100 chars | ✅ Pass | |
| Code compiles independently | ✅ Pass | Appears self-contained |

**Info:** Good fix — propagates actual error codes instead of masking them with `-EINVAL`.

---

## Patch 3/3: `examples/l3fwd: validate port ID in route rules`

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ Pass | ~47 characters |
| Lowercase after colon | ✅ Pass | |
| Correct prefix | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Body wrapped ≤75 chars | ✅ Pass | |
| Signed-off-by present | ✅ Pass | |

### Issues

**Warning — Missing Fixes: tag:** The commit message explicitly states this fixes a bug:
> "The current port ID validation logic... has two issues:
>  - It can pass if port ID in route is 31+."

This is a bug fix and should have a `Fixes:` tag referencing the commit that introduced the flawed port validation logic.

**Warning — Missing Cc: stable@dpdk.org:** Since this fixes a bug (port IDs ≥32 bypass validation), it's a candidate for stable backport.

### Code Review

| Check | Status | Notes |
|-------|--------|-------|
| Lines ≤100 chars | ⚠️ Check | Some lines are borderline |
| Explicit comparisons | ✅ Pass | Uses `== 0`, `!= NULL` style |

**Error — Inconsistent error message format:**

The error messages across files use different formats:

| Location | Message Format |
|----------|----------------|
| l3fwd_em.c:575 | `"IDX: %d: Port ID %d in IPv4 rule..."` |
| l3fwd_em.c:593 | `"IDX %d: Port ID %d given in IPv6 rule..."` |
| l3fwd_fib.c:617 | `"IDX %d: Port ID %d in IPv4 rule..."` |
| l3fwd_lpm.c:661 | `"IDX: %d: Port ID %d in IPv4 rule..."` |

Should standardize to one format (suggest: `"IDX %d: Port ID %d..."`).

**Warning — Comment style:**
```c
bool exit_on_failure; /**< Skip the route rule with invalid or */
		      /**< disabled port ID */
```
Multi-line Doxygen comments should use a single `/** ... */` block, not multiple `/**<` continuations.

**Info — Code duplication:** The `l3fwd_validate_routes_port()` function has significant code duplication between the LPM/FIB and EM branches. Consider refactoring to reduce repetition.

---

## Summary

| Patch | Verdict | Blocking Issues |
|-------|---------|-----------------|
| 1/3 | **Acceptable** | None |
| 2/3 | **Acceptable** | None (minor: terse body) |
| 3/3 | **Needs revision** | Missing Fixes tag, inconsistent error messages |

### Required Changes for Patch 3/3:
1. Add `Fixes:` tag identifying the original commit with the port validation bug
2. Add `Cc: stable@dpdk.org`
3. Standardize error message format across all files

### Recommended Changes:
- Patch 2/3: Expand commit body to explain the impact of the fix
- Patch 3/3: Clean up the multi-line Doxygen comment
- Patch 3/3: Consider refactoring `l3fwd_validate_routes_port()` to reduce duplication