On Fri, 13 Feb 2026 08:46:04 +0100 Lukáš Šišmiš <[email protected]> wrote:
> pá 13. 2. 2026 v 1:43 odesílatel Stephen Hemminger < > [email protected]> napsal: > > > On Fri, 6 Feb 2026 16:40:39 +0100 > > Lukáš Šišmiš <[email protected]> wrote: > > > > > pá 6. 2. 2026 v 15:02 odesílatel Thomas Monjalon > > > <[email protected]> napsal: > > > > > > > 04/02/2026 15:53, Stephen Hemminger: > > > > > On Tue, 3 Feb 2026 09:34:26 +0100 > > > > > Lukáš Šišmiš <[email protected]> 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.
