On Wed, 21 Jan 2026 00:24:17 +0100 Lukas Sismis <[email protected]> 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 <[email protected]> ## 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.
