[PATCH v4 0/7] flow_parser: add shared parser library
Stephen Hemminger
stephen at networkplumber.org
Wed Jan 21 05:55:43 CET 2026
On Wed, 21 Jan 2026 00:24:17 +0100
Lukas Sismis <sismis at 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 at 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.
More information about the dev
mailing list