On Wed, 20 Nov 2024 09:35:13 +0530 Gagandeep Singh <[email protected]> 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: [email protected] | ✅ 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: [email protected]:** 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: [email protected]` 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
