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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  6 Jan 2026 16:38:30 +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:
> - 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@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@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*