Re: [PATCH v5 0/6] flow_parser: add shared parser library

Stephen Hemminger
2026-01-27T19:16:10Z

On Tue, 27 Jan 2026 19:47:31 +0100
Lukas Sismis <sismis@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
> -------
>
> v5:
> - removed/replaced (f)printf code from the library
> - reverted back to exporting the internal/private API as it is needed by
> testpmd and cannot be easily split further.
> - adjusted length of certain lines
> - marking port/queue id typedef as experimental
> - updated release rel_notes
> - copyeright adjustments
>
>
> 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 (6):
> 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
> test: add flow parser library functional tests
>
> 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 | 410 +
> app/test-pmd/flow_parser.h | 12 +
> app/test-pmd/flow_parser_cli.c | 153 +
> 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 | 922 ++
> doc/api/doxy-api-index.md | 1 +
> doc/api/doxy-api.conf.in | 1 +
> doc/guides/prog_guide/flow_parser_lib.rst | 111 +
> doc/guides/prog_guide/index.rst | 1 +
> doc/guides/rel_notes/release_26_03.rst | 21 +
> 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 | 77 +
> lib/flow_parser/meson.build | 7 +
> .../flow_parser/rte_flow_parser.c | 11428 ++++++++--------
> lib/flow_parser/rte_flow_parser.h | 124 +
> lib/flow_parser/rte_flow_parser_private.h | 1247 ++
> lib/meson.build | 2 +
> 29 files changed, 9710 insertions(+), 5833 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_private.h
>

Semi-automated patch review still sees a few things that need to be fixed:

---

## Patch Series Review: [PATCH v5 1/6] - [PATCH v5 6/6] flow parser library

### Overview
This 6-patch series introduces a reusable flow parser library extracted from testpmd's cmdline_flow.c, adds RSS type helper APIs to ethdev, and includes documentation, examples, and tests.

---

### **ERRORS** (must fix)

#### 1. Inconsistent Experimental API Version (Patch 3)
**Location:** `lib/flow_parser/rte_flow_parser.c` lines 15708-15730

The flow_parser library exports symbols with version `26.0`:
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_flow_parser_init, 26.0);
```

But the ethdev patch (2/6) correctly uses `26.03`:
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_rss_type_info_get, 26.03)
```

**Fix:** Change all `26.0` to `26.03` in the flow_parser exports for consistency with the release version.

---

### **WARNINGS** (should fix)

#### 2. Missing Fixes Tag (Patch 1)
**Location:** Commit message of patch 1/6

The commit describes fixing "macro redefinition warnings when building with MSVC" which sounds like a bug fix for existing code. If this is fixing a regression, it should include:
```
Fixes: <12-char-sha> ("original commit that introduced the issue")
Cc: stable@dpdk.org
```

If it's a new enhancement for MSVC support rather than fixing a regression, consider rewording the commit message to make that clearer.

#### 3. Non-Standard Header Guards (Patch 3)
**Location:** `rte_flow_parser.h` and `rte_flow_parser_private.h`

Current:
```c
#ifndef RTE_FLOW_PARSER_H
#define RTE_FLOW_PARSER_H
```

DPDK convention uses underscore prefix/suffix:
```c
#ifndef _RTE_FLOW_PARSER_H_
#define _RTE_FLOW_PARSER_H_
```

#### 4. Implicit Pointer Comparison (Patch 4)
**Location:** `app/test-pmd/flow_parser.c` (around line 17315)

```c
if (!out->args.vc.pattern || out->args.vc.pattern_n == 0)
```

**Fix:** Use explicit comparison per DPDK style:
```c
if (out->args.vc.pattern == NULL || out->args.vc.pattern_n == 0)
```

---

### **INFO** (consider)

#### 5. Test Function Return Type (Patch 2)
**Location:** `app/test/test_ethdev_api.c` line 236

```c
static int32_t
ethdev_api_rss_type_helpers(void)
```

Using `int32_t` for test return types is unusual; `int` is more standard for test framework compatibility.

#### 6. Commit Message Style (Patch 6)
The commit body starts with "Tests cover:" which could be clearer. Consider:
```
Add functional tests for the flow parser library covering:
- success cases...
- fail cases...
```

#### 7. Long String Literal (Patch 6)
**Location:** `app/test/test_flow_parser.c` line 35128

One test string is 106 characters. This is acceptable for test data strings that can't easily be split.

---

### **Positive Observations**

- All commit subject lines are well under 60 characters ✓
- Commit message bodies properly wrapped at 75 characters ✓
- SPDX headers present and correct ✓
- Signed-off-by tags present ✓
- New APIs properly marked as `__rte_experimental` ✓
- Release notes updated in patch 2 and 3 ✓
- Documentation added ✓
- Tests added using proper TEST_ASSERT macros ✓
- Meson build files properly formatted ✓
- No printf/fprintf in library code (only in app/examples) ✓