<div dir="auto">Hi Stephen,<div dir="auto"> </div><div dir="auto">Thanks for the current and earlier feedback. I'll adjust accordingly and send the patches back.</div><div dir="auto"> </div><div dir="auto">Lukas</div></div> <div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, Jan 13, 2026, 18:46 Stephen Hemminger <<a href="mailto:stephen@networkplumber.org">stephen@networkplumber.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, 6 Jan 2026 16:38:30 +0100 Lukas Sismis <<a href="mailto:sismis@dyna-nic.com" target="_blank" rel="noreferrer">sismis@dyna-nic.com</a>> 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] <a href="https://github.com/OISF/suricata/pull/13950" rel="noreferrer noreferrer" target="_blank">https://github.com/OISF/suricata/pull/13950</a> > > 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 <<a href="mailto:sismis@dyna-nic.com" target="_blank" rel="noreferrer">sismis@dyna-nic.com</a>> **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 <<a href="mailto:sismis@dyna-nic.com" target="_blank" rel="noreferrer">sismis@dyna-nic.com</a>> 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* </blockquote></div>