Re: [PATCH v2] app/test-pmd: add generic PROG action parser support

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 21 May 2026 15:43:54 +0530
Megha Ajmera <megha.ajmera@intel.com> 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 <megha.ajmera@intel.com>
> Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> ---

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 <megha.ajmera@intel.com> 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.