On Thu, 21 May 2026 15:43:54 +0530 Megha Ajmera <[email protected]> wrote:
> Add parser support for a generic PROG flow action in testpmd. > > The update adds CLI tokens and parsing logic for program name and > argument tuples (name, size, value), enabling programmable action > configuration through the flow command interface. > > Example flow rule: > flow create 0 ingress pattern eth / end actions prog name my_prog > argument name arg0 size 4 value 10 / end > > Signed-off-by: Megha Ajmera <[email protected]> > Signed-off-by: Praveen Shetty <[email protected]> > --- Ran AI review on this manually since that gets better context. The feedback was: On Thu, 21 May 2026 15:43:54 +0530 Megha Ajmera <[email protected]> wrote: > Add parser support for a generic PROG flow action in testpmd. > > The update adds CLI tokens and parsing logic for program name and > argument tuples (name, size, value), enabling programmable action > configuration through the flow command interface. Errors 1. CREATE proceeds with partially-converted action data on conversion failure. In cmd_flow_parsed() the CREATE case prints a warning if convert_action_prog_to_rte_flow() returns negative, then unconditionally calls port_flow_create(). Inside convert_action_prog_to_rte_flow(), each per-PROG failure does "continue" rather than aborting, so the function returns negative while some actions[i].conf pointers have been replaced with rte_flow_action_prog and others still point to the parser-internal action_prog_data (a completely different layout). The PMD then dereferences a mix of two unrelated structure types. Either abort the create on negative ret, or make convert_action_prog_to_rte_flow() fail-fast: free everything converted so far, restore conf pointers (or never overwrite them until the whole pass succeeds), and return without touching port_flow_create(). 2. Argument value is sent in host byte order, but the API requires network byte order. The doc for struct rte_flow_action_prog_argument in lib/ethdev/rte_flow.h says the value array must be in network byte order. The conversion code does: memcpy(value, &prog_data->args[j].value, args[j].size); prog_data->args[j].value is a host-order uint64_t populated by parse_int. For "size 4 value 10" on little-endian this produces {0x0a,0x00,0x00,0x00}; on big-endian it produces {0x00,0x00,0x00,0x00} -- the leading zero bytes of the 8-byte value, so the user always sees zero. Either convert with rte_cpu_to_be_32/64 before memcpy, or have the parser accept and store the value as a network-order byte array of the declared size. 3. Only the CREATE path is converted; VALIDATE and the async/template paths still pass action_prog_data to the PMD. cmd_flow_parsed() routes the same in->args.vc.actions array to VALIDATE (port_flow_validate), and to multiple async/template paths (port_queue_flow_create, port_flow_template_table_create, port_action_handle_create, etc.). None of these are converted by this patch, so any PROG action used with "flow validate", "flow queue ... create", indirect-action create, or pattern/action templates passes the parser-internal action_prog_data to the PMD with the wrong layout. Conversion needs to be factored into a helper invoked from every code path that hands actions to the ethdev API, not bolted onto CREATE only. 4. Unbounded scan for END in convert_action_prog_to_rte_flow(). while (actions[i].type != RTE_FLOW_ACTION_TYPE_END) i++; If a caller ever passes an actions array without a terminating END, this walks off the end. The caller already knows in->args.vc.actions_n; pass that as the bound and remove the dual-mode behavior (count-or-not). The function is only ever called with prog_action_count == 0, so the other branch is dead code anyway. 5. size == 0 argument is silently accepted but violates the API. The rte_flow_action_prog_argument doc states "its size must be non-zero and its value must point to a valid array of size bytes". The convert code does: args[j].size = prog_data->args[j].size; if (args[j].size == 0) continue; This leaves args[j].value == NULL while args[j].size == 0 and hands that to the PMD. Either reject size == 0 at parse time, or treat it as an error during conversion. Letting it through means valid-looking input produces an API-contract violation that the PMD has no obligation to handle. Warnings 6. Identifiers with a leading double underscore are reserved. __prog_argument_name_args_push, __prog_argument_size_args_push, and __prog_argument_value_args_push use a "__" prefix. The C standard reserves names starting with two underscores for the implementation. Rename to e.g. prog_argument_name_args_push. 7. RTE_SET_USED on parameters that are used. parse_vc_action_prog_argument() marks token and str as unused, then passes both to parse_default(). parse_vc_action_prog_argument_value() marks size as unused, then passes it to parse_vc_conf(). The v2 changelog says "Fixed compilation warning"; these spurious RTE_SET_USED calls look like that fix. Remove them -- they mislead future readers and can hide a real unused parameter if one is added later. 8. Doxygen parameter name mismatch. The comment block above convert_action_prog_to_rte_flow() documents "@param action_count" but the actual parameter is named prog_action_count. The same comment describes a dual-mode interface ("If 0, will count until END action") that is never exercised -- fix the name or simplify the interface. 9. No testpmd user-guide update. This adds new CLI syntax actions prog name <name> argument name <n> size <s> value <v> but doc/guides/testpmd_app_ug/testpmd_funcs.rst is not updated. Users have no documented way to learn the syntax. 10. No release notes update. A new user-visible testpmd feature should have a one-line entry in the current release notes. Info 11. The static "struct arg arg[ACTION_PROG_MAX_ARGS]" arrays inside the three __prog_argument_*_args_push helpers can simply be a single non-static "struct arg" (or three) on the stack. push_args() consumes the entries during the current parse_vc_conf() call; persistent storage across CLI invocations is not needed and obscures the lifetime. 12. action_prog_data.length is written by the COMMON_STRING parser for the prog name field but never read afterwards (convert uses strdup, relying on NUL termination). It is required as a destination for the COMMON_STRING args triple, but a brief comment would prevent the next reader from thinking it is meaningful state.
