Re: [PATCH v10 3/6] flow_parser: add shared parser library

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 13 Feb 2026 08:46:04 +0100
Lukáš Šišmiš <sismis@dyna-nic.com> wrote:

> pá 13. 2. 2026 v 1:43 odesílatel Stephen Hemminger <
> stephen@networkplumber.org> napsal:  
> 
> > On Fri, 6 Feb 2026 16:40:39 +0100
> > Lukáš Šišmiš <sismis@dyna-nic.com> wrote:
> >  
> > > pá 6. 2. 2026 v 15:02 odesílatel Thomas Monjalon
> > > <thomas@monjalon.net> napsal:
> > >  
> > > > 04/02/2026 15:53, Stephen Hemminger:  
> > > > > On Tue, 3 Feb 2026 09:34:26 +0100
> > > > > Lukáš Šišmiš <sismis@dyna-nic.com> wrote:
> > > > >  
> > > > > > >
> > > > > > > The kernel version of checkpatch complains here. The DPDK
> > > > > > > shell  
> > > > script  
> > > > > > > seems to be set to ignore this but.
> > > > > > >
> > > > > > > WARNING: break is not useful after a return
> > > > > > > #15008: FILE: lib/flow_parser/rte_flow_parser.c:14763:
> > > > > > > +               return cmd_flow_parsed(out);
> > > > > > > +               break;
> > > > > > >
> > > > > > > Should I create a new patch set or just let it be at this
> > > > > > >  
> > moment?  
> > > > > > Lukas  
> > > > >
> > > > >
> > > > > I am ok with it as is.  
> > > >
> > > > Better to update.
> > > >
> > > > There are other warnings:
> > > >
> > > > WARNING:STRNCPY: Prefer strscpy, strscpy_pad, or __nonstring
> > > > over  
> > strncpy  
> > > > - see: https://github.com/KSPP/linux/issues/90
> > > > #13052 <https://github.com/KSPP/linux/issues/90#13052>: FILE:
> > > > lib/flow_parser/rte_flow_parser.c:12825:
> > > > +       strncpy(buf, str, len);
> > > >
> > > > and a lot of WARNING:LONG_LINE
> > > >
> > > > I can have a look after the decision.  
> > >  
> > > >
> > > > And on a more general note, I would have expected to ask the
> > > > opinion of rte_flow maintainers, but they are not Cc'ed in
> > > > these patches. 
> > > I communicated primarily with Stephen, and will CC Ori too.
> > > Anyone else?
> > >
> > >  
> > > > I'm a bit skeptical about adding this outside of ethdev library
> > > > which defines the flow API.
> > > >  
> > >
> > > CCing Ori to make a decision. I don't mind putting it directly
> > > into  
> > ethdev  
> > > as well, I just thought the parser could be its own separate lib
> > > as it is just consuming strings and producing rte_flow
> > > structures. I can see the heavy ties to the flow library, though.
> > > Thomas, Stephen, what are your opinions?  
> >
> > I am beginning to agree with Thomas, this belongs in the ethdev
> > directory next to rte_flow.
> >
> > Also, not sure what the purpose of parser_ops is. It says to pass
> > NULL, but test-pmd is doing something else. It would be better to
> > have an initializer (i.e RTE_INIT) instead? Maybe
> >  
> 
> 
> Ok, I can move it there then. Should I proceed with Patch v11 - that
> would include moving the ethdev directory and addressing the kernel's
> checkpatch issues?
> 
> The initializer doesn't seem like a bad idea. I didn't know about
> that one. The parser_ops are not needed for purely parsing, but to
> remain compatible with the testpmd's code, which is using, e.g., the
> hints on the commandline I added parser_ops.
> The simple public library API is for converting strings to rte_flow
> structures,
> the parser_ops is for hooking up the testpmd.


My response was more immediate (gut feel). But wanted more detailed
feedback.
I think you need to have a really simple API like the PCAP
does for compiling string into BPF. See pcap_compile.

The testpmd/cmdline method is creating too much baggage!

This is what the result of back-and-forth with Claude AI came to:

rte_flow_parser_ops — Summary of Issues

64 callbacks, zero documentation. No Doxygen on any member. The only
way to understand the contract is to read testpmd's implementation —
making testpmd the de facto spec.

Inconsistent return types. 36 callbacks return int, 10 return void.
Similar operations use different conventions (e.g. flow_create returns
int, flow_tunnel_create returns void). No error reporting possible from
the void ones.

Testpmd internals leaked into the API. verbose_level_get is a UI
concern, not a parser concern. The void/int split mirrors whatever
testpmd's internal functions happened to use, not a designed interface.

No guidance on minimal implementation. "Unused callbacks may be NULL"
but which ones does the parser actually call during a basic flow
create? A new consumer can't tell without tracing the code.

ABI-fragile but installed as a header. Adding any new rte_flow command
means extending these structs. No versioning mechanism, yet it's in
indirect_headers so external code can (and will) use it.

681-entry command enum couples consumers to parser internals. Anyone
interpreting rte_flow_parser_output.command must understand the full
token namespace.

Bottom line: This is a testpmd function dispatch table promoted to a
library header without the design work to make it an external API.
Either don't install the private header, or invest in
documenting/versioning it properly.