Re: [PATCH v4 0/7] flow_parser: add shared parser library

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 21 Jan 2026 00:24:17 +0100
Lukas Sismis <sismis@dyna-nic.com> wrote:

> This series extracts the testpmd flow CLI parser into a reusable library,
> enabling external applications to parse rte_flow rules using testpmd syntax.
> 
> Motivation
> ----------
> External applications like Suricata IDS [1] need to express hardware filtering
> rules in a consistent, human-readable format. Rather than inventing custom
> syntax, reusing testpmd's well-tested flow grammar provides immediate
> compatibility with existing documentation and user knowledge.
> 
> Note: This library provides only one way to create rte_flow structures.
> Applications can also construct rte_flow_attr, rte_flow_item[], and
> rte_flow_action[] directly in C code.
> 
> Design
> ------
> The library (librte_flow_parser) exposes the following APIs:
> - rte_flow_parser_parse_attr_str(): Parse attributes only
> - rte_flow_parser_parse_pattern_str(): Parse patterns only
> - rte_flow_parser_parse_actions_str(): Parse actions only
> 
> Testpmd is updated to use the library, ensuring a single
> maintained parser implementation.
> 
> Testing and Demo
> -------
> - Functional tests in dpdk-test
> - Example application: examples/flow_parsing
> 
> Changes
> -------
> 
> v4:
> - ethdev changes in separate commit
> - library's public API only exposes attribute, pattern and action parsing,
>   while the full command parsing is kept internal for testpmd usage only.
> - Addressed Stephen's comments from V3
> - dpdk-test now have tests focused on public and internal library functions
> 
> v3:
> - Add more functional tests
> - More concise MAINTAINERS updates
> - Updated license headers
> - A thing to note: When playing with flow commands, I figured, some may use
>   non-flow commands, such as raw decap/encap, policy meter and others.
>   Flow parser library itself now supports `set` command to set e.g. the decap/
>   encap parameters, as the flow syntax only supports defining the index of the
>   encap/decap configs. The library, however, does not support e.g. `create`
>   command to create policy meters, as that is just an ID and it can be created
>   separately using rte_meter APIs.
> 
> [1] https://github.com/OISF/suricata/pull/13950
> 
> 
> Lukas Sismis (7):
>   cmdline: include stddef.h for offsetof
>   ethdev: add RSS type helper APIs
>   flow_parser: add shared parser library
>   app/testpmd: use shared flow parser library
>   examples/flow_parsing: add flow parser demo
>   dpdk-test: add flow parser library functional tests
>   mailmap: update a new contributor email
> 
>  .mailmap                                      |     2 +-
>  MAINTAINERS                                   |     6 +-
>  app/test-pmd/cmd_flex_item.c                  |    41 +-
>  app/test-pmd/cmdline.c                        |   268 +-
>  app/test-pmd/config.c                         |   112 +-
>  app/test-pmd/flow_parser.c                    |   406 +
>  app/test-pmd/flow_parser.h                    |     8 +
>  app/test-pmd/flow_parser_cli.c                |   149 +
>  app/test-pmd/meson.build                      |     5 +-
>  app/test-pmd/testpmd.c                        |     4 +
>  app/test-pmd/testpmd.h                        |   126 +-
>  app/test/meson.build                          |     1 +
>  app/test/test_ethdev_api.c                    |    51 +
>  app/test/test_flow_parser.c                   |   923 ++
>  doc/api/doxy-api-index.md                     |     1 +
>  doc/api/doxy-api.conf.in                      |     1 +
>  doc/guides/prog_guide/flow_parser_lib.rst     |   115 +
>  doc/guides/prog_guide/index.rst               |     1 +
>  doc/guides/rel_notes/release_26_03.rst        |     9 +
>  examples/flow_parsing/main.c                  |   291 +
>  examples/flow_parsing/meson.build             |    11 +
>  examples/meson.build                          |     1 +
>  lib/cmdline/cmdline_parse.h                   |     2 +
>  lib/ethdev/rte_ethdev.c                       |   107 +
>  lib/ethdev/rte_ethdev.h                       |    60 +
>  lib/flow_parser/meson.build                   |     7 +
>  .../flow_parser/rte_flow_parser.c             | 11377 ++++++++--------
>  lib/flow_parser/rte_flow_parser.h             |   124 +
>  lib/flow_parser/rte_flow_parser_internal.h    |  1236 ++
>  lib/meson.build                               |     2 +
>  30 files changed, 9642 insertions(+), 5805 deletions(-)
>  create mode 100644 app/test-pmd/flow_parser.c
>  create mode 100644 app/test-pmd/flow_parser.h
>  create mode 100644 app/test-pmd/flow_parser_cli.c
>  create mode 100644 app/test/test_flow_parser.c
>  create mode 100644 doc/guides/prog_guide/flow_parser_lib.rst
>  create mode 100644 examples/flow_parsing/main.c
>  create mode 100644 examples/flow_parsing/meson.build
>  create mode 100644 lib/flow_parser/meson.build
>  rename app/test-pmd/cmdline_flow.c => lib/flow_parser/rte_flow_parser.c (55%)
>  create mode 100644 lib/flow_parser/rte_flow_parser.h
>  create mode 100644 lib/flow_parser/rte_flow_parser_internal.h
> 

AI review spotted some issues as well.
The one that matters is:

Fix fprintf/printf usage in lib/flow_parser/rte_flow_parser.c
This is the most critical issue
Replace all fprintf/printf with proper error handling
Consider using RTE_LOG() for any necessary logging
Return appropriate error codes to caller

Please cleanup and resubmit

Full report:
# DPDK Flow Parser v4 Patch Series Review

**Reviewer:** Stephen Hemminger  
**Date:** January 20, 2026  
**Series:** [PATCH v4 1/7] - [PATCH v4 7/7] Flow Parser Library  
**Submitter:** Lukas Sismis <sismis@dyna-nic.com>

## Executive Summary

This 7-patch series introduces a new experimental `librte_flow_parser` library that extracts testpmd's flow CLI parser into a reusable component. The series is generally well-structured and follows most DPDK conventions, but has several **warnings** that should be addressed before merging.

**Overall Assessment:** Good work with notable issues to address

---

## Patch-by-Patch Review

### Patch 1/7: cmdline: include stddef.h for offsetof
**Status:** ✅ GOOD

**Commit Message:**
- Subject line: 38 chars ✓
- Format: Correct prefix, lowercase, no trailing period ✓
- Body: Clear explanation of the fix
- Signed-off-by: Present ✓

**Code:**
- SPDX identifier would be present in existing file ✓
- Simple, correct fix for MSVC compatibility

**Verdict:** Ready to merge

---

### Patch 2/7: ethdev: add RSS type helper APIs
**Status:** ⚠️ NEEDS ATTENTION

**Commit Message:**
- Subject line: 32 chars ✓
- Format: Correct ✓
- Missing: Release notes update for new APIs

**Code Review:**
✅ **Good:**
- SPDX identifiers present
- APIs properly marked `__rte_experimental` ✓
- Unit tests added (`test_ethdev_api.c`) ✓
- Clean API design with string conversion helpers

⚠️ **Warning - Release Notes:**
The patch introduces new public APIs (`rte_eth_rss_type_*` functions and new typedefs) but these are not documented in the release notes. Only the flow_parser library is mentioned in patch 3's release notes.

**Required Action:**
Add a release notes entry for the new ethdev RSS type helper APIs in this patch.

---

### Patch 3/7: flow_parser: add shared parser library
**Status:** ⚠️ MULTIPLE WARNINGS

**Commit Message:**
- Subject line: 38 chars ✓
- Format: Correct ✓
- Body: Comprehensive description ✓
- Signed-off-by: Present ✓

**License & Headers:**
✅ SPDX identifiers present in all new files:
- `lib/flow_parser/rte_flow_parser.h`
- `lib/flow_parser/rte_flow_parser.c`
- `lib/flow_parser/rte_flow_parser_internal.h`

**API Design:**
✅ All public APIs properly marked `__rte_experimental`:
- `rte_flow_parser_init()`
- `rte_flow_parser_parse_attr_str()`
- `rte_flow_parser_parse_pattern_str()`
- `rte_flow_parser_parse_actions_str()`

**Documentation:**
✅ Comprehensive documentation added:
- New guide: `doc/guides/prog_guide/flow_parser_lib.rst`
- Release notes updated
- API documentation in header

**Meson Build Files:**
✅ Style compliant:
- 4-space indentation
- Alphabetically ordered dependencies

**Code Quality Issues:**

⚠️ **WARNING - fprintf/printf in Library Code (High Priority)**

Found **74 instances** of `fprintf()` and `printf()` in `lib/flow_parser/rte_flow_parser.c`:

Examples:
```c
Line 2678: fprintf(stderr, "Error - set_raw_encap failed for index %u\n", index);
Line 2688: fprintf(stderr, "Error - set_raw_decap failed for index %u\n", index);
Line 10817: printf("Bad flex item handle\n");
Line 11863: fprintf(stderr, "Error - raw decap index %u is empty\n", idx);
```

**From AGENTS.md Section 4.3:**
> "Use of `perror()`, `printf()`, `fprintf()` in libraries or drivers (allowed in examples and test code)"

**Recommendation:** 
This is a library, not test/example code. These should be replaced with:
1. Return error codes instead of printing
2. Use RTE_LOG() macros for logging if necessary
3. Let the calling application handle error reporting

This is the most significant issue in the series and should be fixed before merging.

⚠️ **INFO - Line Length (Minor)**

Found **12 lines** exceeding 100 characters:
```
Line 2422: 111 chars - Macro definition
Line 4791: 101 chars - Parser state machine
Line 5434: 114 chars - Parser command list
```

While these slightly exceed the limit, they're in auto-generated or state machine code where breaking them might reduce readability. Consider if these can be reasonably refactored, but this is low priority compared to the fprintf issue.

---

### Patch 4/7: app/testpmd: use shared flow parser library
**Status:** ✅ GOOD

**Commit Message:**
- Subject line: 43 chars ✓
- Format: Correct ✓
- Comprehensive description of changes ✓

**Code:**
- Removes duplicate code (cmdline_flow.c) ✓
- Adds proper bridge layer (flow_parser.c, flow_parser_cli.c) ✓
- MAINTAINERS file updated ✓
- SPDX identifiers in new files ✓

**Architecture:**
Good separation of concerns - testpmd now acts as a client of the library rather than embedding the parser.

**Verdict:** Well-executed refactoring

---

### Patch 5/7: examples/flow_parsing: add flow parser demo
**Status:** ✅ GOOD

**Commit Message:**
- Subject line: 43 chars ✓
- Format: Correct ✓

**Code:**
- SPDX identifier present ✓
- Example demonstrates library usage ✓
- MAINTAINERS updated ✓
- Meson build file properly formatted ✓

**Note:** Since this is example code, any printf usage here is acceptable.

**Verdict:** Good example code

---

### Patch 6/7: dpdk-test: add flow parser library functional tests  
**Status:** ⚠️ MINOR ISSUE

**Commit Message:**
- Subject line: 51 chars ✓
- **Prefix Issue:** Uses `dpdk-test:` instead of `test:` ⚠️

**From AGENTS.md:**
```
| Wrong       | Correct  |
|-------------|----------|
| app/test:   | test:    |
```

While `dpdk-test:` isn't explicitly listed, the convention is that the test directory should use `test:` as the prefix.

**Recommended Subject:**
```
test: add flow parser library functional tests
```

**Code Review:**
✅ **Good:**
- SPDX identifier present ✓
- Comprehensive test coverage ✓
- **Proper use of TEST_ASSERT macros** ✓
- **Uses unit_test_suite_runner infrastructure** ✓
- REGISTER_FAST_TEST() macro used correctly ✓
- Tests both success and failure cases ✓

**Test Suite Structure:**
```c
static struct unit_test_suite flow_parser_tests = {
    .suite_name = "flow parser autotest",
    .setup = flow_parser_setup,
    .teardown = flow_parser_teardown,
    .unit_test_cases = {
        TEST_CASE_ST(flow_parser_case_setup, NULL, test_flow_parser_public_attr_parsing),
        // ... more tests ...
        TEST_CASES_END()
    }
};
```

This follows DPDK test infrastructure best practices exactly as specified in AGENTS.md Section 3.3.

**Verdict:** Excellent test infrastructure usage, minor commit message prefix issue

---

### Patch 7/7: mailmap: update a new contributor email
**Status:** ✅ GOOD

**Commit Message:**
- Subject line: 39 chars ✓
- Format: Correct ✓
- Signed-off-by: Present ✓

**Code:**
- Simple mailmap update ✓

**Verdict:** Standard administrative change

---

## Summary of Issues

### 🔴 HIGH PRIORITY - Must Fix

1. **Patch 3 - fprintf/printf in Library Code**
   - **Severity:** WARNING (High Priority)
   - **Location:** `lib/flow_parser/rte_flow_parser.c` (74 instances)
   - **Issue:** Libraries should not use fprintf/printf for error reporting
   - **Action Required:** Replace with proper error codes or RTE_LOG()
   - **Blocking:** Yes - this violates library design principles

### 🟡 MEDIUM PRIORITY - Should Fix

2. **Patch 2 - Missing Release Notes for ethdev APIs**
   - **Severity:** WARNING
   - **Issue:** New ethdev APIs not documented in release notes
   - **Action Required:** Add release notes entry for RSS helper APIs

3. **Patch 6 - Incorrect Commit Prefix**
   - **Severity:** INFO
   - **Issue:** Uses `dpdk-test:` instead of `test:`
   - **Action Required:** Change subject line prefix

### 🟢 LOW PRIORITY - Consider

4. **Patch 3 - Line Length**
   - **Severity:** INFO
   - **Issue:** 12 lines exceed 100 characters
   - **Action Required:** Review if reasonable to refactor
   - **Note:** May be acceptable given context (state machine code)

---

## Validation Checklist Results

### Commit Messages
- [x] All subject lines ≤60 characters
- [x] All subjects are lowercase (except acronyms)
- [x] Correct component prefixes (with 1 minor issue in patch 6)
- [x] Imperative mood, no trailing periods
- [x] Body text wrapped at 75 characters
- [x] Signed-off-by present on all patches
- [x] No "It" at start of commit body
- [ ] Release notes complete for all API changes (Missing in patch 2)

### License
- [x] SPDX identifiers present on all new files
- [x] Copyright lines follow SPDX
- [x] Appropriate license (BSD-3-Clause) used

### Code Style
- [x] Hard tabs for indentation
- [x] No trailing whitespace (assumed, not explicitly checked)
- [x] Header guards present
- [x] `rte_` prefix on external symbols
- [x] Proper brace style
- [ ] Lines ≤100 characters (12 minor overruns)

### API Design
- [x] New APIs marked `__rte_experimental`
- [x] `__rte_experimental` alone on line, only in headers
- [x] No ABI compatibility concerns (experimental APIs)

### Documentation
- [x] Programmer's guide added
- [x] API documentation in headers
- [x] Release notes updated (flow_parser)
- [ ] Release notes complete (missing ethdev APIs)

### Testing
- [x] Unit tests added for new library
- [x] Tests use TEST_ASSERT macros properly
- [x] Tests use unit_test_suite_runner infrastructure
- [x] REGISTER_FAST_TEST used correctly
- [x] Example application provided

### Build System
- [x] Meson files properly formatted
- [x] Dependencies declared correctly
- [x] Each patch compiles independently (assumed)

---

## Recommendations

### Before Merge - Required Changes:

1. **Fix fprintf/printf usage in lib/flow_parser/rte_flow_parser.c**
   - This is the most critical issue
   - Replace all fprintf/printf with proper error handling
   - Consider using RTE_LOG() for any necessary logging
   - Return appropriate error codes to caller

2. **Add release notes for ethdev RSS APIs (Patch 2)**
   - Document the new `rte_eth_rss_type_*` functions
   - Document the new typedefs (rte_port_id_t, rte_queue_id_t)

3. **Fix commit prefix in Patch 6**
   - Change `dpdk-test:` to `test:`

### Nice to Have:

4. **Review line length overruns**
   - 12 lines slightly exceed 100 chars
   - Consider refactoring if reasonable
   - Document why if they need to stay

---

## Overall Assessment

This is a **well-structured patch series** that makes good architectural sense. The extraction of testpmd's flow parser into a reusable library is valuable for the DPDK ecosystem. The code demonstrates:

**Strengths:**
- Clean API design with experimental tagging
- Comprehensive documentation
- Excellent test coverage following best practices
- Good example application
- Proper meson build integration
- Thoughtful refactoring of testpmd

**Weaknesses:**
- Inappropriate use of fprintf/printf in library code (must fix)
- Incomplete release notes (should fix)
- Minor commit message prefix issue (easy fix)

The fprintf/printf issue is the only blocking concern. This appears to be carried over from the testpmd code and needs to be addressed to follow DPDK library design principles.

**Recommendation:** Request v5 with the fprintf/printf usage fixed and release notes completed.

---

## Additional Comments

The test infrastructure usage in patch 6 is **exemplary** and exactly follows DPDK best practices. The author clearly understands the unit_test_suite_runner framework and TEST_ASSERT macro usage.

The documentation in patch 3 is comprehensive and will help users adopt this new library.

This series represents significant effort and value to the DPDK project. With the identified issues addressed, it will be a solid contribution.