[PWCI] "flow_parser: add shared parser library"#598
Conversation
Introduce librte_flow_parser as an optional, experimental library exposing the testpmd flow CLI parser as a reusable component. The library provides: - rte_flow_parser_init(): Initialize parser with operation callbacks - rte_flow_parser_parse(): Parse flow command strings into output - rte_flow_parser_run(): Parse and execute via registered callbacks - Helper functions for parsing pattern/action/attribute strings The parser uses a single global instance design for simplicity. All parsing state is internal to the library. Callbacks are invoked for flow create, destroy, validate, query and other flow operations. This enables applications to reuse testpmd's well-tested flow syntax without duplicating the parser implementation. Signed-off-by: Lukas Sismis <sismis@dyna-nic.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Switch testpmd to use the shared librte_flow_parser library instead of the embedded cmdline_flow.c implementation. Changes: - Remove app/test-pmd/cmdline_flow.c (moved to library) - Add flow_parser.c: Implements flow operation callbacks that bridge the parser library to testpmd's port/flow management functions - Add flow_parser_cli.c: Hosts cmdline integration for 'flow' and 'set raw_encap/decap' commands - Update cmdline.c to register flow commands with the parser - Update meson.build to link against flow_parser library The testpmd flow command syntax remains unchanged. All existing flow commands continue to work identically. Signed-off-by: Lukas Sismis <sismis@dyna-nic.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Add example application demonstrating the flow parser library API. The example shows: - Using parse_attr_str() to parse flow attributes from strings - Using parse_pattern_str() to parse match patterns - Using parse_actions_str() to parse flow actions - Printing parsed results with RTE_FLOW_LOG macros - Integration without EAL initialization (library is standalone) Build with: meson configure -Dexamples=flow_parsing Signed-off-by: Lukas Sismis <sismis@dyna-nic.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Add comprehensive unit tests for the flow parser library.
Tests cover:
- flow_parser_autotest: Tests rte_flow_parser_parse() with various
flow command strings including create, destroy, validate, query,
list, and isolate commands. Verifies parsed patterns, actions,
and attributes match expected values.
- flow_parser_helpers_autotest: Tests the standalone helper functions
parse_attr_str(), parse_pattern_str(), and parse_actions_str().
Verifies parsing of attributes, patterns with various item types,
and action sequences.
Run with: dpdk-test flow_parser_autotest
dpdk-test flow_parser_helpers_autotest
Signed-off-by: Lukas Sismis <sismis@dyna-nic.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Add maintainer entries for the new flow parser library. Update .mailmap with new contributor email. Signed-off-by: Lukas Sismis <sismis@dyna-nic.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Fix Python syntax errors in f-strings that used nested quotes
incorrectly. The pattern f'{foo['bar']}' is invalid Python 3.11
syntax and breaks Sphinx autodoc during documentation builds.
Use f"{foo['bar']}" with outer double quotes instead.
Signed-off-by: Lukas Sismis <sismis@dyna-nic.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Include <stddef.h> at the top of cmdline_parse.h before the fallback offsetof macro definition. This prevents macro redefinition warnings when building with MSVC which uses /WX (warnings as errors). The standard header provides offsetof on all platforms, and including it first ensures the fallback is only used when truly needed. Signed-off-by: Lukas Sismis <sismis@dyna-nic.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ovsrobot, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughIntroduces a new experimental Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant testpmd as testpmd<br/>CLI
participant parser as rte_flow_parser<br/>(Library)
participant adapter as testpmd<br/>Adapter<br/>(flow_parser.c)
participant handlers as Port<br/>Handlers
participant ethdev as DPDK<br/>Core
User->>testpmd: "flow create 0 pattern eth / actions drop"
testpmd->>parser: rte_flow_parser_parse(cmd_str)
parser->>parser: Tokenize & validate
parser-->>testpmd: rte_flow_parser_output
testpmd->>adapter: Query ops (validate_flow, create_flow)
adapter->>handlers: port_flow_validate() / port_flow_create()
handlers->>ethdev: rte_flow_create()
ethdev-->>handlers: flow_handle
handlers-->>adapter: result
adapter-->>testpmd: status
testpmd-->>User: Success/Error message
Note over parser,adapter: New architecture:<br/>Parsing decoupled from<br/>execution via callbacks
sequenceDiagram
actor App as External App
participant init as Initialization
participant parser as rte_flow_parser
participant parse_fn as Parse Functions
participant helpers as Lightweight<br/>Helpers
App->>init: testpmd_flow_parser_init()
init->>parser: rte_flow_parser_init(ops_table)
parser->>parser: Register callback ops
parser-->>init: Success
App->>parse_fn: rte_flow_parser_parse(pattern_str)
parse_fn->>parse_fn: Full parse + execute
parse_fn-->>App: rte_flow_parser_output
alt Lightweight (no execution)
App->>helpers: rte_flow_parser_parse_attr_str()
helpers->>helpers: Parse only
helpers-->>App: rte_flow_attr
end
Note over parser,helpers: Two modes:<br/>Full (with dispatch)<br/>Lightweight (parse-only)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @lib/flow_parser/rte_flow_parser.h:
- Line 208: Fix the spelling typo in the comment that reads "Sub-leve commands."
by updating it to "Sub-level commands." so the descriptive comment for the
sub-level command section is correct; locate the comment string in
lib/flow_parser/rte_flow_parser.h (the line containing "Sub-leve commands.") and
replace it accordingly.
🧹 Nitpick comments (12)
lib/ethdev/rte_ethdev.c (1)
5171-5276: RSS type mapping API is sound; clarify 0/NULL semantics and intended usageThe static
rte_eth_rss_type_tableand the three helpers are straightforward and match typical DPDK patterns; no correctness issues jump out. Two behavioral points worth calling out:
rte_eth_rss_type_from_str()returns0both for"none"and for NULL/unknown input. That’s fine if call‑sites either special‑case"none"or treat “unknown” as “no bits”, but it does make error vs “none” indistinguishable from the return value alone.rte_eth_rss_type_to_str()only returns a name for exact entries in the table; combined bitmasks will correctly yieldNULL. It might be useful to explicitly document that it’s for single/group constants, not arbitrary OR’d masks.If these semantics are what the higher‑level CLI/parser already expects, I’d just ensure they’re documented in
rte_ethdev.hso users don’t misinterpret 0/NULL as hard errors.app/test-pmd/cmd_flex_item.c (1)
132-133: Consider heap allocation for large parser output buffer.The 4096-byte stack allocation for
outbufmay be excessive for environments with limited stack space. Consider using heap allocation if this function is called in contexts where stack space is constrained.💡 Alternative approach using heap allocation
- uint8_t outbuf[4096]; + uint8_t *outbuf = malloc(4096); struct rte_flow_parser_output *out = (void *)outbuf; int ret; + + if (!outbuf) + return -ENOMEM; sprintf(flow_rule, "flow create 0 pattern %s / end actions drop / end", src); memset(outbuf, 0, sizeof(outbuf)); ret = rte_flow_parser_parse(flow_rule, out, sizeof(outbuf)); - if (ret) + if (ret) { + free(outbuf); return ret; + } if (out->command != RTE_FLOW_PARSER_CMD_CREATE) + { + free(outbuf); return -EINVAL; + } + // ... rest of the function ... + free(outbuf);lib/ethdev/rte_ethdev.h (1)
171-174: RSS helper APIs and new typedefs look consistent; consider clarifying “not found” semanticsThe
rte_port_id_t/rte_queue_id_ttypedefs are straightforward aliases and align with existing usage of 16‑bit IDs, and the RSS type helper trio (_info_get,_from_str,_to_str) cleanly exposes the static table fromrte_ethdev.c.One small design point:
rte_eth_rss_type_from_str()returns 0 both for “not found/NULL” and for a legitimate value if 0 were ever introduced as a valid RSS type. Today allRTE_ETH_RSS_*bits are non‑zero, so it’s safe, but you may want to document this explicitly or consider a futurebool/intreturn plus an out‑param if an actual 0-valued type is ever added.Also applies to: 4897-4950
app/test-pmd/flow_parser.c (3)
21-38: Index-based helpers are simple but give O(n²) traversal when enumerating many flows
parser_flow_rule_count()walksport->flow_listonce, andparser_flow_rule_id_get()walks from the head again for each index viaparser_flow_by_index(). If the parser library iteratesindex=0..count-1, total cost is O(n²) in the number of rules.Given this is testpmd/CLI-oriented it’s probably fine, but if you expect very large rule lists it might be worth either:
- Storing a count in
struct rte_portand returning it directly, and/or- Adding a direct “nth element” helper that caches traversal state when called repeatedly.
Also applies to: 81-92
40-58: Template/table enumeration uses the same O(n²) pattern as flows
parser_pattern_template_count/_id_get,parser_actions_template_count/_id_get, andparser_table_count/_id_getmirror the flow helpers and re-traverse the linked lists from the head for every index.As with flows, this is acceptable for small lists and CLI usage, but will degrade quadratically if the library walks all indices. If these lists can grow large, consider either caching counts on
struct rte_portand/or adopting an iterator pattern rather than “count + indexed get”.Also applies to: 110-154, 172-199
201-220: RSS queue count fallback logic: clarify assumptions ondev_info.max_rx_queues
parser_rss_queue_count()returnsport->queue_nbwhen queues are configured, otherwise falls back toport->dev_info.max_rx_queues. That makes sense for pre‑configuration introspection, but relies ondev_infohaving been populated andmax_rx_queuesbeing non‑zero.If there is any path where
parser_rss_queue_count()can be called beforedev_infois valid, you may want a defensive fallback (e.g., return 0) or a brief comment stating the assumption thatdev_infohas already been filled.app/test-pmd/flow_parser_cli.c (2)
13-33: Dynamic token wiring viacl == NULLis unconventional; ensure call sites match the contract
cmd_flow_cb/cmd_set_raw_cbtreatcl == NULLas “token registration” (delegating torte_flow_parser_cmd_*_tok) andcl != NULLas “dispatch” (delegating torte_flow_parser_cmd_*_dispatch). The associatedcmdline_parse_inst_tinstances (cmd_flow,cmd_set_raw) start with.tokens = { NULL }, so some initialization path must be explicitly invoking these callbacks withcl == NULLto fill in the token arrays.This is a clever reuse of the
fcallback, but it’s also non‑standard for cmdline. Please double‑check that your initialization code actually callscmd_flow_cb(NULL, …)/cmd_set_raw_cb(NULL, …)with appropriate arguments so that the tokens get populated before the CLI starts parsing commands.Also applies to: 109-125
34-94:show raw_encap/raw_decaphelper is robust; minor robustness nits onlyThe
cmd_show_set_raw_parsed()logic correctly:
- Distinguishes
allvs single index usingcmd_all,- Validates
cmd_indexagainstRAW_ENCAP_CONFS_MAX_NUM,- Checks
conf/data/sizebefore dumping, and- Iterates over all configured entries when requested.
A couple of small, optional refinements:
- Since
cmd_show_set_raw_parsedis shared between the<index>andallvariants, it relies on the parser zero‑initializingstruct cmd_show_set_raw_result, socmd_allis an empty string for the<index>form. That’s how cmdline works today, but adding an explicitif (res->cmd_all[0] == '\0')comment or check would make the dependency clearer.title[16]is tight but safe for current index ranges; bumping to something like 32 bytes would leave more headroom ifRAW_ENCAP_CONFS_MAX_NUMever grows significantly.None of this blocks the change; the current implementation is functionally sound.
Also applies to: 96-149
app/test-pmd/cmdline.c (2)
10559-11279: Encap/decap “set” commands correctly write into librte_flow_parser config structsThe refactor of
set vxlan/nvgre/l2_encap/l2_decap/mplsogre_encap/mplsogre_decap/mplsoudp_encap/mplsoudp_decapto:
- fetch
struct rte_flow_parser_*_conf *via the correspondingrte_flow_parser_*_encap_conf()/*_decap_conf()helpers,- NULL‑check those pointers before touching them, and
- populate fields (VNI/TNI/labels, UDP ports, IPs, VLAN TCI, MACs, selector bits)
matches the previous per‑feature CLI behavior while moving state into the shared parser library. The use of
IPV4_ADDR_TO_UINT/IPV6_ADDR_TO_ARRAYandrte_cpu_to_be_16/32keeps the expected endianness. The only small polish you might consider is logging a brief error instead of silentlyreturnwhen a*_conf()helper returns NULL, to aid debugging if the parser isn’t initialized.
11457-11624: Defensively NULL‑check rte_flow_parser_conntrack_context() before useIn
cmd_set_conntrack_common_parsedandcmd_set_conntrack_dir_parsedyou now obtain the conntrack context viarte_flow_parser_conntrack_context()but don’t NULL‑check it before dereferencing, unlike the otherrte_flow_parser_*_conf()helpers above. If this ever failed (e.g., parser not initialized), the CLI would segfault.Consider aligning with the other helpers and returning early on NULL, e.g.:
Proposed defensive checks
static void cmd_set_conntrack_common_parsed(void *parsed_result, __rte_unused struct cmdline *cl, __rte_unused void *data) { struct cmd_set_conntrack_common_result *res = parsed_result; - struct rte_flow_action_conntrack *ct = rte_flow_parser_conntrack_context(); + struct rte_flow_action_conntrack *ct = rte_flow_parser_conntrack_context(); + + if (ct == NULL) + return; @@ static void cmd_set_conntrack_dir_parsed(void *parsed_result, __rte_unused struct cmdline *cl, __rte_unused void *data) { struct cmd_set_conntrack_dir_result *res = parsed_result; - struct rte_flow_action_conntrack *ct = rte_flow_parser_conntrack_context(); + struct rte_flow_action_conntrack *ct = rte_flow_parser_conntrack_context(); struct rte_flow_tcp_dir_param *dir = NULL; + + if (ct == NULL) + return;This keeps the CLI robust even if the global parser context isn’t available for some reason.
lib/flow_parser/rte_flow_parser.h (2)
934-936: Inconsistent comment style for struct members.Lines 935-936 use
/**but end with just*/without proper Doxygen format. Consider using consistent/**< ... */style for trailing member comments.🔎 Proposed fix
enum rte_flow_parser_command_index command; /**< Flow command. */ uint16_t port; /**< Affected port ID. */ - uint16_t queue; /** Async queue ID. */ - bool postpone; /** Postpone async operation */ + uint16_t queue; /**< Async queue ID. */ + bool postpone; /**< Postpone async operation. */ union {
1244-1298: Document thread-safety constraints more prominently.The helper functions (
rte_flow_parser_parse_attr_str,rte_flow_parser_parse_pattern_str,rte_flow_parser_parse_actions_str) return pointers to internal storage that is invalidated by the next call. While this is documented per-function, consider adding a prominent note at the top of the API section about the parser using global/thread-local state, which has implications for concurrent usage.Based on the external context showing a single global
parserinstance withparser.ctx, concurrent calls from multiple threads could lead to data races. If thread-safety is required, consider documenting this limitation explicitly or using thread-local storage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.mailmapMAINTAINERSapp/test-pmd/cmd_flex_item.capp/test-pmd/cmdline.capp/test-pmd/config.capp/test-pmd/flow_parser.capp/test-pmd/flow_parser.happ/test-pmd/flow_parser_cli.capp/test-pmd/meson.buildapp/test-pmd/testpmd.capp/test-pmd/testpmd.happ/test/meson.buildapp/test/test_flow_parser.cdoc/guides/prog_guide/flow_parser_lib.rstdoc/guides/prog_guide/index.rstdoc/guides/rel_notes/release_26_03.rstdts/api/testpmd/__init__.pyexamples/flow_parsing/main.cexamples/flow_parsing/meson.buildexamples/meson.buildlib/cmdline/cmdline_parse.hlib/ethdev/rte_ethdev.clib/ethdev/rte_ethdev.hlib/flow_parser/meson.buildlib/flow_parser/rte_flow_parser.clib/flow_parser/rte_flow_parser.hlib/meson.build
🧰 Additional context used
🧬 Code graph analysis (10)
dts/api/testpmd/__init__.py (2)
dts/framework/testbed_model/os_session.py (1)
send_command(134-163)dts/framework/exception.py (1)
InteractiveCommandExecutionError(180-184)
app/test-pmd/testpmd.c (1)
app/test-pmd/flow_parser.c (1)
testpmd_flow_parser_init(402-406)
lib/ethdev/rte_ethdev.h (1)
lib/ethdev/rte_ethdev.c (3)
rte_eth_rss_type_info_get(5241-5245)rte_eth_rss_type_from_str(5248-5262)rte_eth_rss_type_to_str(5265-5276)
app/test-pmd/cmd_flex_item.c (1)
lib/flow_parser/rte_flow_parser.c (1)
rte_flow_parser_parse(14690-14718)
examples/flow_parsing/main.c (1)
lib/flow_parser/rte_flow_parser.c (4)
rte_flow_parser_parse_attr_str(14803-14822)rte_flow_parser_parse_pattern_str(14824-14846)rte_flow_parser_parse_actions_str(14848-14872)rte_flow_parser_init(1070-1076)
app/test-pmd/config.c (2)
lib/flow_parser/rte_flow_parser.c (2)
p(13612-13616)rte_flow_parser_conntrack_context(1126-1130)lib/ethdev/rte_ethdev.c (3)
rte_eth_rss_type_to_str(5265-5276)rte_eth_rss_type_info_get(5241-5245)rte_eth_rss_type_from_str(5248-5262)
app/test-pmd/testpmd.h (1)
app/test-pmd/flow_parser.c (1)
testpmd_flow_parser_init(402-406)
app/test/test_flow_parser.c (1)
lib/flow_parser/rte_flow_parser.c (5)
rte_flow_parser_init(1070-1076)rte_flow_parser_parse(14690-14718)rte_flow_parser_parse_attr_str(14803-14822)rte_flow_parser_parse_pattern_str(14824-14846)rte_flow_parser_parse_actions_str(14848-14872)
app/test-pmd/flow_parser_cli.c (1)
lib/flow_parser/rte_flow_parser.c (6)
rte_flow_parser_cmd_flow_tok(14660-14665)rte_flow_parser_cmd_flow_dispatch(14674-14680)rte_flow_parser_cmd_set_raw_tok(14667-14672)rte_flow_parser_cmd_set_raw_dispatch(14682-14688)rte_flow_parser_raw_encap_conf_get(821-832)rte_flow_parser_raw_decap_conf_get(834-844)
lib/flow_parser/rte_flow_parser.h (1)
lib/flow_parser/rte_flow_parser.c (19)
rte_flow_parser_reset_defaults(1056-1068)rte_flow_parser_vxlan_encap_conf(1078-1082)rte_flow_parser_nvgre_encap_conf(1084-1088)rte_flow_parser_l2_encap_conf(1090-1094)rte_flow_parser_l2_decap_conf(1096-1100)rte_flow_parser_mplsogre_encap_conf(1102-1106)rte_flow_parser_mplsogre_decap_conf(1108-1112)rte_flow_parser_mplsoudp_encap_conf(1114-1118)rte_flow_parser_mplsoudp_decap_conf(1120-1124)rte_flow_parser_conntrack_context(1126-1130)rte_flow_parser_raw_encap_conf_get(821-832)rte_flow_parser_raw_decap_conf_get(834-844)rte_flow_parser_init(1070-1076)rte_flow_parser_cmdline_register(14652-14658)rte_flow_parser_parse(14690-14718)rte_flow_parser_run(14720-14745)rte_flow_parser_parse_attr_str(14803-14822)rte_flow_parser_parse_pattern_str(14824-14846)rte_flow_parser_parse_actions_str(14848-14872)
🪛 Cppcheck (2.19.0)
app/test-pmd/flow_parser.c
[error] 361-361: syntax error
(syntaxError)
🔇 Additional comments (46)
.mailmap (1)
936-936: Correctly consolidated contributor identity mapping.The mailmap entry properly consolidates both email addresses for Lukas Sismis, with the newer affiliation (sismis@dyna-nic.com) as the canonical identity while preserving the legacy alias. The entry is correctly positioned alphabetically and follows the standard
.mailmapformat used elsewhere in the file.dts/api/testpmd/__init__.py (3)
230-230: LGTM - Style consistency improvement.The change from
""to''is purely cosmetic and improves quote style consistency within the f-string. No functional impact.
901-905: LGTM - Style consistency improvement.The changes standardize quote usage within f-string conditional expressions (double quotes → single quotes). These are purely stylistic with no functional impact on the error logging or exception messages.
1538-1538: LGTM - Style consistency improvement.The change standardizes quote usage within the f-string conditional expression (double quotes → single quotes). The testpmd command string remains functionally identical.
MAINTAINERS (1)
445-452: Flow API ownership for new flow parser bits looks consistentListing
lib/flow_parser/,app/test-pmd/flow_parser.c, and the newflow_parser_libdoc under the Flow API section matches the new library and its integration; nothing else to flag here.lib/cmdline/cmdline_parse.h (1)
10-18: Including<stddef.h>before the fallbackoffsetofmacro is correctPulling in
<stddef.h>and only definingoffsetofunder#ifndef offsetofis the right way to avoid compiler warnings while preserving the existing fallback; no further changes needed.app/test/meson.build (1)
90-92: test_flow_parser meson wiring matches new library dependenciesHooking
test_flow_parser.cintosource_file_depswith['flow_parser', 'cmdline', 'ethdev']aligns with the new library’s usage and integrates cleanly with the existing per‑file dependency logic; looks good.doc/guides/prog_guide/index.rst (1)
120-124: Prog guide index correctly lists the new Flow Parser libraryAdding
flow_parser_libto the Utility Libraries toctree is consistent with the new documentation file and keeps ordering sensible; no issues here.app/test-pmd/meson.build (1)
16-17: LGTM: Flow parser integration into testpmd build.The addition of
flow_parser.candflow_parser_cli.csource files along with theflow_parserlibrary dependency correctly integrates the new flow parser functionality into the testpmd build system.Also applies to: 38-38
examples/flow_parsing/meson.build (1)
9-11: LGTM: Standard example build configuration.The build configuration correctly enables experimental APIs and declares the dependency on the
flow_parserlibrary, following standard DPDK example patterns.app/test-pmd/testpmd.c (1)
4619-4621: LGTM: Flow parser initialization with appropriate error handling.The flow parser initialization is correctly placed in the startup sequence (after
init_config()but before device operations) and includes proper error handling withrte_exit()on failure. The initialization ensures the parser is ready before any flow operations can be performed.app/test-pmd/flow_parser.h (1)
1-8: LGTM: Minimal header exposing flow parser API.The header correctly includes the flow parser public API and follows standard DPDK header conventions with appropriate include guards.
examples/meson.build (1)
20-20: LGTM: Example correctly added to build list.The
flow_parsingexample is correctly added to the alphabetically sortedall_exampleslist.lib/flow_parser/meson.build (1)
1-6: LGTM! Build configuration is correct.The Meson build file properly defines the flow parser library with appropriate dependencies on
cmdlineandethdev.lib/meson.build (2)
71-71: LGTM! Library registration is correct.The
flow_parserlibrary is properly added to the libraries list. The placement at the end is appropriate for a new library that depends on existing core libraries.
79-79: LGTM! Always-enable configuration is appropriate.Adding
flow_parserto thealways_enablelist ensures it's always built and available, which is appropriate for a library that provides essential flow parsing functionality.doc/guides/rel_notes/release_26_03.rst (1)
58-65: LGTM! Release notes are clear and comprehensive.The release notes accurately describe the new experimental flow parser library, its purpose, and key features. The documentation properly emphasizes the experimental and optional nature of the library.
doc/guides/prog_guide/flow_parser_lib.rst (1)
1-140: LGTM! Excellent comprehensive documentation.The documentation thoroughly covers the flow parser library with:
- Clear explanation of purpose and alternatives
- Important thread-safety and buffer management caveats
- Comprehensive API coverage
- Usage examples and build instructions
- Callback model explanation
The documentation properly warns users about thread-safety limitations and buffer size requirements, which are critical for correct usage.
examples/flow_parsing/main.c (4)
37-163: LGTM! Helper functions are well-implemented.The helper functions properly:
- Check for null specs/configs before dereferencing
- Use appropriate buffer sizes for inet_ntop
- Apply correct byte-order conversions with ntohs
- Handle multiple pattern and action types
272-320: Good documentation of buffer limitations.The
demo_combinefunction excellently documents the static buffer limitation (lines 266-269 and 283), warning users that returned pointers are only valid until the next parse call. The comments at lines 316-319 appropriately suggest copying data in real applications.This is a clear and helpful demonstration of proper usage patterns.
322-345: LGTM! Main function structure is clean and correct.The main function properly:
- Initializes the flow parser with appropriate error checking
- Runs all demonstration functions in logical order
- Returns correct exit codes
5-25: Excellent header documentation.The file header provides outstanding context by:
- Clearly stating this is ONE WAY to create rte_flow structures
- Documenting alternative approaches (direct C construction)
- Explaining appropriate use cases
- Listing the key functions demonstrated
This helps users understand when to use the flow parser vs. other approaches.
app/test/test_flow_parser.c (2)
11-88: Good test coverage for command parsing.The test thoroughly validates the flow parser's ability to parse different command types and extract the correct information from the parsed output. The validation checks are comprehensive.
90-220: Excellent coverage of helper APIs.The test thoroughly exercises the attribute, pattern, and action parsing helpers with various configurations. The validation of queue configurations and mark IDs demonstrates proper testing of parsed action parameters.
app/test-pmd/config.c (4)
667-697: Good migration to ethdev RSS type API.The refactor to use
rte_eth_rss_type_to_str()instead of a local static mapping improves maintainability and ensures consistency with the ethdev library's RSS type naming.
1447-1473: Well-refactored RSS types display using runtime API.The migration from a static table to using
rte_eth_rss_type_info_get()improves maintainability. The iteration logic correctly filters out invalid entries and matches enabled types.
4718-4748: Appropriate migration to ethdev RSS type conversion API.The change from
str_to_rsstypes()torte_eth_rss_type_from_str()correctly adopts the ethdev library's standard API for RSS type string conversion, improving code consistency.
1768-1778: No issues found. The conntrack context initialization is handled correctly at application startup viatestpmd_flow_parser_init()called in testpmd.c before any code in config.c executes. Therte_flow_parser_conntrack_context()function is the official API for accessing the parser-maintained conntrack context, and its usage is consistent throughout the codebase (cmdline.c uses the same pattern). The design aligns with the flow parser library's intent to maintain global state for conntrack and other configuration contexts.app/test-pmd/cmd_flex_item.c (1)
149-169: Use pattern length field to avoid unnecessary copying of zero-padded buffer.The memcpy operations at lines 151-152, 158-159, and 165-166 copy a fixed
FLEX_MAX_FLOW_PATTERN_LENGTHbytes, but the source pattern buffers have alengthfield that indicates the actual valid data size (populated at cmd_flex_item.c:505). While the source buffers are zero-initialized, copying 64 bytes when the pattern may be only 10-20 bytes is inefficient and masks the intent of the code.Update the memcpy calls to respect the actual pattern length:
if (out->args.vc.pattern[0].spec) { ptr = (void *)(uintptr_t)item->spec; memcpy(ptr, out->args.vc.pattern[0].spec, out->args.vc.pattern[0].spec->length); }Or ensure the contract is documented: if buffers are guaranteed to be exactly
FLEX_MAX_FLOW_PATTERN_LENGTHbytes with appropriate padding, document that assumption clearly.app/test-pmd/flow_parser.c (3)
60-73: Stack-basedtunnel_opsconversion relies on callees not retaining the pointer
parser_tunnel_convert()fills a caller-providedstruct tunnel_opson the stack and returnsdst. Callers likeparser_flow_validate,parser_flow_create, andparser_flow_tunnel_createimmediately pass that pointer intoport_flow_*functions.This is fine as long as the
port_flow_*implementations only readtunnel_opsduring the call and do not store the pointer for later asynchronous use. If any of those functions cache the pointer inside a long-lived object, this would become a use‑after‑return bug.Please double-check that
port_flow_validate(),port_flow_create(), andport_flow_tunnel_create()treattunnel_opsas strictly call-local data.Also applies to: 282-307, 316-324
253-274: Flex handle / pattern getters are defensively implemented
parser_flex_handle_get()andparser_flex_pattern_get()both guard against out-of-range IDs and NULLs, and cleanly return NULL /-ENOENTwithout dereferencing invalid pointers. The pattern getter also returns pointers into the staticflex_patternsstorage, so the caller need not manage lifetime.This looks solid and matches expected semantics for optional resources.
326-345: Ops tables wiring looks correct; Cppcheck “syntax error” is likely a false positiveThe
parser_query_ops,parser_command_ops, andparser_opsinitializers consistently map to the correspondingrte_flow_parser_ops_*fields, and the function signatures match what the names suggest.The line split around
.flow_template_table_resize_complete =followed on the next line byport_flow_template_table_resize_complete,is valid C and should not be a real syntax issue. The reported Cppcheck “syntax error” around this region is almost certainly a parser limitation on designated initializers or macros rather than a true problem.No change needed functionally; if you want to quiet that specific tool you could place the initializer on one line, but it’s not required.
Also applies to: 347-400, 402-405
app/test-pmd/cmdline.c (3)
39-40: Flow parser headers and local integration look consistentIncluding
<rte_flow_parser.h>and the new"flow_parser.h"here is the right place, and ordering with other rte headers is consistent with the rest of testpmd. This should give you the needed prototypes and local helpers without introducing layering issues, assuming the build linkslibrte_flow_parseron all relevant targets.Also applies to: 69-70
2362-2367: RSS configuration now uses rte_eth_rss_type_from_str – semantics look correctSwitching to
rte_eth_rss_type_from_str(res->value)centralizes the string→RSS-hf mapping and will automatically pick up new types defined by ethdev. The 0‑return check with “Unknown parameter” is a clear error path, andnone/numeric ids are still handled earlier in the function, so behavior for existing CLI values should be preserved. Please just double‑check against the ethdev API docs that no valid RSS type ever maps to0in your target DPDK version.
9457-9460: Dynamic flow/“set raw” CLI integration with librte_flow_parser is wired correctlyDeclaring the new extern
cmd_set_raw,cmd_show_set_raw, andcmd_show_set_raw_all, adding them tobuiltin_ctx, and registering dynamic flow/set tokens viarte_flow_parser_cmdline_register(&cmd_flow, &cmd_set_raw);ininit_cmdline()ties the shared parser library cleanly into the existing CLI. The registration happens beforemain_ctxis built and before any CLI interaction, which is the right place.Just ensure that:
cmd_set_raw/cmd_show_set_raw*are always linked in when testpmd is built with flow parser support, andrte_flow_parser_cmdline_register()is safe to call exactly once from this init path (and is a no‑op or clearly documented if the library is disabled at build time).Functionally this layout looks sound.
Also applies to: 14359-14361, 14390-14392
app/test-pmd/testpmd.h (3)
19-19: LGTM!The new include adds the flow parser library header, enabling access to the parser API and types.
837-837: LGTM!The function prototype correctly declares
testpmd_flow_parser_initwhich, per the external context, wrapsrte_flow_parser_init(&parser_ops)inapp/test-pmd/flow_parser.c.
98-99: No action needed. Bothrte_port_id_tandrte_queue_id_tare properly defined inrte_ethdev.h(at lines 172 and 174 respectively), which is already included at line 17 of testpmd.h. The typedefs are correct and have all necessary dependencies.lib/flow_parser/rte_flow_parser.h (8)
1-17: LGTM!The header has proper SPDX license, include guards, necessary standard headers, and C++ extern "C" linkage for compatibility.
19-29: LGTM!The shared limit constants define reasonable bounds for buffer sizes and array limits.
35-51: LGTM!The VXLAN encap configuration structure includes all necessary fields with appropriate types and bit-fields for optional features.
116-157: LGTM!The accessor functions provide consistent access to mutable encap/decap configurations and raw encap/decap data. The
constreturn type forrte_flow_parser_raw_encap_conf_getandrte_flow_parser_raw_decap_conf_getcorrectly indicates that the returned action configs should not be modified.
161-167: LGTM!The tunnel ops structure captures the necessary tunnel steering/match flags with appropriate bit-fields.
1026-1056: LGTM!The query ops structure provides comprehensive hooks for the parser to validate IDs and fetch cached objects. The callbacks allow applications to customize ID resolution and object lookup.
1062-1173: LGTM!The command ops structure provides comprehensive hooks for all flow CLI operations. The callbacks are appropriately optional as documented.
1180-1189: LGTM!The
rte_flow_parser_initfunction is well-documented, clearly stating the ops parameter may be NULL and returns 0 on success or negative errno on error.
|
|
||
| /* Top-level command. */ | ||
| RTE_FLOW_PARSER_CMD_SET, | ||
| /* Sub-leve commands. */ |
There was a problem hiding this comment.
Typo: "Sub-leve" should be "Sub-level".
🔎 Proposed fix
- /* Sub-leve commands. */
+ /* Sub-level commands. */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Sub-leve commands. */ | |
| /* Sub-level commands. */ |
🤖 Prompt for AI Agents
In @lib/flow_parser/rte_flow_parser.h at line 208, Fix the spelling typo in the
comment that reads "Sub-leve commands." by updating it to "Sub-level commands."
so the descriptive comment for the sub-level command section is correct; locate
the comment string in lib/flow_parser/rte_flow_parser.h (the line containing
"Sub-leve commands.") and replace it accordingly.
1 participant
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36972"
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.