Hi Stephen, Thanks for the current and earlier feedback. I'll adjust accordingly and send the patches back.
Lukas On Tue, Jan 13, 2026, 18:46 Stephen Hemminger <[email protected]> wrote: > On Tue, 6 Jan 2026 16:38:30 +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: > > - rte_flow_parser_parse(): Parse command strings into structured output > > - 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 shared library, ensuring a single > > maintained parser implementation. > > > > Testing > > ------- > > - Unit tests: flow_parser_autotest, flow_parser_helpers_autotest > > - Example application: examples/flow_parsing > > - All existing testpmd flow commands work unchanged > > > > > > > > [1] https://github.com/OISF/suricata/pull/13950 > > > > Lukas Sismis (7): > > flow_parser: add shared parser library > > app/testpmd: integrate shared flow parser library > > examples/flow_parsing: add flow parser demo > > test: add flow parser unit tests > > doc: add flow parser library maintainers > > dts: fix invalid f-string syntax in testpmd API > > cmdline: include stddef.h before defining offsetof > > > > .mailmap | 2 +- > > MAINTAINERS | 3 + > > app/test-pmd/cmd_flex_item.c | 41 +- > > app/test-pmd/cmdline.c | 254 +- > > 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_flow_parser.c | 226 + > > doc/guides/prog_guide/flow_parser_lib.rst | 140 + > > doc/guides/prog_guide/index.rst | 1 + > > doc/guides/rel_notes/release_26_03.rst | 9 + > > dts/api/testpmd/__init__.py | 8 +- > > examples/flow_parsing/main.c | 345 + > > 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 | 6 + > > .../flow_parser/rte_flow_parser.c | 10774 ++++++++-------- > > lib/flow_parser/rte_flow_parser.h | 1304 ++ > > lib/meson.build | 2 + > > 27 files changed, 8600 insertions(+), 5507 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 > (57%) > > create mode 100644 lib/flow_parser/rte_flow_parser.h > > > > Looks OK to me, so asked AI for second opinion. > It saw some things that do need to be fixed. > > > # DPDK Patch Review: Flow Parser Library (v2) > > **Series:** `[PATCH v2 0/7]` flow_parser: add shared parser library > **Author:** Lukas Sismis <[email protected]> > **Date:** Tue, 6 Jan 2026 > **Reviewer:** AI Code Review Tool > > --- > > ## Series Overview > > This patch series introduces `librte_flow_parser` as an optional, > experimental library that exposes the testpmd flow CLI parser as a reusable > component. The series is well-structured: > > | Patch | Description | > |-------|-------------| > | 1/7 | Core library implementation (~15K lines) | > | 2/7 | testpmd integration - removes embedded parser, uses library | > | 3/7 | Example application demonstrating library usage | > | 4/7 | Unit tests for the parser | > | 5/7 | MAINTAINERS and .mailmap updates | > | 6/7 | DTS f-string syntax fix (tangential bugfix) | > | 7/7 | cmdline stddef.h include fix (tangential bugfix) | > > --- > > ## Errors (Must Fix) > > ### 1. Missing Copyright Lines in New Files > > **Severity:** Error > **Rule:** SPDX license identifier must be followed by copyright notice > **Location:** Multiple files > > Per DPDK guidelines, every source file must have SPDX identifier followed > by copyright: > > **`lib/flow_parser/rte_flow_parser.h`** (Patch 1): > ```c > /* SPDX-License-Identifier: BSD-3-Clause */ > #ifndef RTE_FLOW_PARSER_H > ``` > Missing: `Copyright(c) 2026 Dyna-NIC` line after SPDX > > **`app/test/test_flow_parser.c`** (Patch 4): > ```c > /* SPDX-License-Identifier: BSD-3-Clause */ > > #include <stdint.h> > ``` > Missing: Copyright line after SPDX > > **Recommended fix:** > ```c > /* SPDX-License-Identifier: BSD-3-Clause > * Copyright(c) 2026 Dyna-NIC > */ > ``` > > --- > > ### 2. Subject Lines Exceed 60 Characters > > **Severity:** Error > **Rule:** Subject line maximum 60 characters > **Location:** Patches 2 and 7 > > | Patch | Length | Subject | > |-------|--------|---------| > | 2/7 | **64** | `[PATCH v2 2/7] app/testpmd: integrate shared flow parser > library` | > | 7/7 | **65** | `[PATCH v2 7/7] cmdline: include stddef.h before defining > offsetof` | > > **Recommended fixes:** > - Patch 2: `app/testpmd: use shared flow parser library` (47 chars with > prefix) > - Patch 7: `cmdline: include stddef.h for offsetof` (54 chars with prefix) > > --- > > ## Warnings (Should Fix) > > ### 3. Implicit Pointer/Integer Comparisons > > **Severity:** Warning > **Rule:** Compare pointers explicitly with NULL, integers with zero > **Location:** `lib/flow_parser/rte_flow_parser.c` > > Multiple instances use `!ptr` instead of `ptr == NULL`: > > ```c > // Current (non-compliant): > if (!data) > if (!ipv4->version_ihl) > if (!ops || !ops->port_validate) > if (!conf->select_ipv4) > > // Should be: > if (data == NULL) > if (ipv4->version_ihl == 0) > if (ops == NULL || ops->port_validate == NULL) > if (conf->select_ipv4 == 0) > ``` > > Affected locations include lines: 767, 783, 943, 999, 1012, 1053, 1091, > 1104, 1167, 1381, 1390, 1393, 1658, 1668, 1678, 1688, 1699, 1709, 1720, > 1730, and many more. > > --- > > ### 4. Missing Newline at End of File (Cross-Patch Fix) > > **Severity:** Warning > **Rule:** Files must end with a newline > **Location:** `app/test-pmd/flow_parser.h` > > Patch 2 creates this file without a trailing newline: > ``` > -#endif /* _TESTPMD_FLOW_PARSER_H_ */ > \ No newline at end of file > ``` > > Patch 3 fixes it, but each commit should compile independently. The fix > should be in Patch 2. > > --- > > ### 5. MAINTAINERS Entry Missing Explicit Maintainer > > **Severity:** Warning > **Rule:** New subsystems should have release notes and maintainer entries > **Location:** `MAINTAINERS` > > The update adds files to the Flow API section: > ``` > F: lib/flow_parser/ > F: app/test-pmd/flow_parser.c > F: doc/guides/prog_guide/flow_parser_lib.rst > ``` > > However, no explicit `M:` (maintainer) entry is added. Consider adding: > ``` > Flow Parser Library > M: Lukas Sismis <[email protected]> > F: lib/flow_parser/ > F: doc/guides/prog_guide/flow_parser_lib.rst > ``` > > --- > > ## Info (Consider) > > ### 6. Unrelated Fixes Bundled in Series > > Patches 6/7 and 7/7 fix issues unrelated to the flow parser: > - Patch 6: DTS f-string syntax fix > - Patch 7: cmdline stddef.h include > > These should ideally be separate patch submissions with their own `Fixes:` > tags if they address regressions. If they're prerequisites for the flow > parser, this should be documented. > > --- > > ### 7. Code Structure Positive Notes > > The series demonstrates good practices: > > ✓ New APIs marked as `__rte_experimental` (correctly on separate lines in > headers) > ✓ Release notes updated (`doc/guides/rel_notes/release_26_03.rst`) > ✓ Documentation added (`doc/guides/prog_guide/flow_parser_lib.rst`) > ✓ Unit tests added (`app/test/test_flow_parser.c`) > ✓ Example application provided (`examples/flow_parsing/`) > ✓ Proper use of `RTE_EXPORT_EXPERIMENTAL_SYMBOL` > ✓ Thread-local storage for helper functions > ✓ Commit messages don't start with "It" > > --- > > ## Checklist Summary > > | Category | Status | > |----------|--------| > | SPDX + Copyright | ❌ Missing copyright in 2 files | > | Subject line ≤60 chars | ❌ 2 patches exceed limit | > | Subject lowercase (except acronyms) | ✓ | > | Correct component prefix | ✓ | > | Body ≤75 chars | ✓ | > | Signed-off-by present | ✓ | > | __rte_experimental placement | ✓ | > | New APIs experimental | ✓ | > | Release notes | ✓ | > | Documentation | ✓ | > | Tests added | ✓ | > | Explicit comparisons | ❌ Multiple implicit comparisons | > | Each commit compiles | ⚠️ Missing newline issue | > > --- > > ## Recommended Actions for v3 > > 1. **Add copyright lines** to `rte_flow_parser.h` and `test_flow_parser.c` > 2. **Shorten subject lines** for patches 2 and 7 > 3. **Fix newline** at end of `flow_parser.h` in patch 2 (not patch 3) > 4. **Consider** adding explicit `M:` entry in MAINTAINERS > 5. **Consider** splitting patches 6-7 into separate series or adding > dependency notes > 6. **Consider** converting implicit comparisons to explicit style (lower > priority, many occurrences) > > --- > > *Review generated based on DPDK AGENTS.md guidelines* > > >
