[RFC PATCH v2 0/7] flow_parser: add shared parser library

Lukáš Šišmiš sismis at dyna-nic.com
Thu Jan 15 10:01:15 CET 2026
Previous message (by thread): [RFC PATCH v2 0/7] flow_parser: add shared parser library
Next message (by thread): [PATCH v5 1/4] vhost: add user to mailmap and define to vhost hdr
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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 <stephen at networkplumber.org>
wrote:

> On Tue,  6 Jan 2026 16:38:30 +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:
> > - 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 <sismis at dyna-nic.com>
> **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 <sismis at dyna-nic.com>
> 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*
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20260115/633f68d2/attachment-0001.htm>
Previous message (by thread): [RFC PATCH v2 0/7] flow_parser: add shared parser library
Next message (by thread): [PATCH v5 1/4] vhost: add user to mailmap and define to vhost hdr
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list