|SUCCESS| [v2] app/test-pmd: add generic PROG action parser support

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164226

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-21

Reviewing patch: add generic PROG action parser support

commit sha: 20260521101354.726240-1-megha.ajmera at intel.com
Author: Megha Ajmera <megha.ajmera at intel.com>
Subject: app/test-pmd: add generic PROG action parser support

This patch adds parser support for a generic PROG flow action in testpmd,
enabling programmable action configuration through the flow command
interface.

> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index ebc036b14b..daf1de7146 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c

[ ... ]

> @@ -981,6 +988,24 @@ struct action_sample_data {
>  	struct rte_flow_action_sample conf;
>  	uint32_t idx;
>  };
> +
> +#define ACTION_PROG_MAX_ARGS 32
> +#define ACTION_PROG_NAME_LEN_MAX 64
> +#define ACTION_PROG_ARG_NAME_LEN_MAX 16
> +
> +struct action_prog_argument_data {
> +	char name[ACTION_PROG_ARG_NAME_LEN_MAX];
> +	uint32_t length;
> +	uint32_t size;
> +	uint64_t value;
> +};
> +
> +struct action_prog_data {
> +	char name[ACTION_PROG_NAME_LEN_MAX];
> +	uint32_t args_num;
> +	uint32_t length;
> +	struct action_prog_argument_data args[ACTION_PROG_MAX_ARGS];
> +};

Does the 'length' field in action_prog_argument_data serve any purpose?
It appears to be set but never used in the conversion or validation logic.

[ ... ]

> +/** Parse PROG action */
> +static int
> +parse_vc_action_prog(struct context *ctx, const struct token *token,
> +			const char *str, unsigned int len, void *buf,
> +			unsigned int size)
> +{
> +	struct buffer *out = buf;
> +	struct action_prog_data *prog_data;
> +	int ret;
> +
> +	ret = parse_vc(ctx, token, str, len, buf, size);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Nothing else to do if there is no buffer. */
> +	if (!out)
> +		return ret;
> +
> +	if (!out->args.vc.actions_n)
> +		return -1;

Does this code check that out->args.vc.data is non-NULL before using it
as ctx->object? The parse_vc_action_sample() function in the same file
performs this validation.

> +
> +	/* Point to selected object. */
> +	ctx->object = out->args.vc.data;
> +	ctx->objmask = NULL;
> +
> +	prog_data = ctx->object;
> +	prog_data->args_num = 0;
> +
> +	return ret;
> +}

[ ... ]

> +static int __prog_argument_name_args_push(struct context *ctx, const struct token *token,
> +					  const char *str, unsigned int len, void *buf,
> +					  unsigned int size, uint32_t arg_ind)

In __prog_argument_name_args_push(), __prog_argument_size_args_push(),
and __prog_argument_value_args_push(), are the static arrays arg_addr[],
arg_len[], arg_data[], and arg[] thread-safe? Can testpmd's flow command
parsing be invoked concurrently from multiple threads?

> +{
> +	static struct arg arg_addr[ACTION_PROG_MAX_ARGS];
> +	static struct arg arg_len[ACTION_PROG_MAX_ARGS];
> +	static struct arg arg_data[ACTION_PROG_MAX_ARGS];

[ ... ]

> +static int
> +convert_action_prog_to_rte_flow(struct rte_flow_action *actions,
> +				uint32_t prog_action_count,
> +				uint32_t *converted_idx,
> +				uint32_t converted_idx_cap,
> +				uint32_t *converted_idx_num)
> +{
> +	uint32_t i = 0;
> +	uint32_t j;
> +	uint32_t k;
> +	const struct action_prog_data *prog_data;
> +	struct rte_flow_action_prog *prog;
> +	struct rte_flow_action_prog_argument *args;
> +	uint8_t *value;
> +	bool arg_error;
> +	int ret = 0;
> +
> +	if (converted_idx_num)
> +		*converted_idx_num = 0;
> +
> +	/* If action_count is 0, count the actions until END action */
> +	if (prog_action_count == 0) {
> +		while (actions[i].type != RTE_FLOW_ACTION_TYPE_END)
> +			i++;
> +		prog_action_count = i + 1; /* Include END action */
> +		i = 0; /* Reset counter for the processing loop */
> +	}

Does this loop have bounds checking for the actions array? Can an
unterminated actions array cause this loop to overflow?

> +
> +	/* Process all actions */
> +	for (i = 0; i < prog_action_count; i++) {
> +		if (actions[i].type == RTE_FLOW_ACTION_TYPE_PROG) {
> +			prog_data = (const struct action_prog_data *)actions[i].conf;
> +			if (!prog_data) {
> +				fprintf(stderr, "Prog action found but no data provided\n");
> +				ret = -EINVAL;
> +				continue;
> +			}
> +
> +			prog = calloc(1, sizeof(struct rte_flow_action_prog));
> +			if (!prog) {
> +				fprintf(stderr, "Failed to allocate memory for prog action\n");
> +				ret = -ENOMEM;
> +				continue;
> +			}
> +
> +			prog->name = strdup(prog_data->name);
> +			if (!prog->name) {
> +				fprintf(stderr, "Failed to allocate memory for prog name\n");
> +				free(prog);
> +				ret = -ENOMEM;
> +				continue;
> +			}

In convert_action_prog_to_rte_flow(), when allocation failures occur
midway through the loop and ret is set to -ENOMEM or -EINVAL with
continue, does this leave previously converted actions without cleanup?
The function returns ret but previously allocated resources from earlier
loop iterations remain in actions[].conf without being freed.

> +
> +			prog->args_num = prog_data->args_num;
> +
> +			if (prog->args_num > 0) {
> +				args = calloc(prog->args_num,
> +					      sizeof(struct rte_flow_action_prog_argument));
> +				if (!args) {
> +					fprintf(stderr,
> +						"Failed to allocate memory for prog arguments\n");
> +					free((void *)(uintptr_t)prog->name);
> +					free(prog);
> +					ret = -ENOMEM;
> +					continue;
> +				}
> +
> +				arg_error = false;
> +				for (j = 0; j < prog->args_num; j++) {
> +					args[j].name = strdup(prog_data->args[j].name);
> +					if (!args[j].name) {
> +						fprintf(stderr,
> +							"Failed to allocate memory for argument name\n");
> +						ret = -ENOMEM;
> +						arg_error = true;
> +						break;
> +					}
> +
> +					args[j].size = prog_data->args[j].size;
> +					if (args[j].size == 0)
> +						continue;
> +					if (args[j].size > sizeof(prog_data->args[j].value)) {
> +						free((void *)(uintptr_t)args[j].name);
> +						arg_error = true;
> +						ret = -EINVAL;
> +						break;
> +					}
> +
> +					value = malloc(args[j].size);
> +					if (!value) {
> +						fprintf(stderr,
> +							"Failed to allocate memory for argument value\n");
> +						free((void *)(uintptr_t)args[j].name);
> +						ret = -ENOMEM;
> +						arg_error = true;
> +						break;
> +					}
> +
> +					memcpy(value,
> +					       &prog_data->args[j].value,
> +					       args[j].size);
> +					args[j].value = value;
> +				}
> +
> +				if (arg_error) {
> +					/* Free all allocated resources */
> +					for (k = 0; k < j; k++) {
> +						free((void *)(uintptr_t)args[k].name);
> +						free((void *)(uintptr_t)args[k].value);
> +					}
> +					free(args);
> +					free((void *)(uintptr_t)prog->name);
> +					free(prog);
> +					continue;
> +				}
> +
> +				prog->args = args;
> +			}
> +
> +			actions[i].conf = prog;
> +			if (converted_idx && converted_idx_num &&
> +			    *converted_idx_num < converted_idx_cap)
> +				converted_idx[(*converted_idx_num)++] = i;
> +		}
> +	}
> +
> +	return ret;
> +}

[ ... ]

> @@ -13561,9 +14112,36 @@ cmd_flow_parsed(const struct buffer *in)
>  				   &in->args.vc.tunnel_ops);
>  		break;
>  	case CREATE:
> -		port_flow_create(in->port, &in->args.vc.attr,
> -				 in->args.vc.pattern, in->args.vc.actions,
> -				 &in->args.vc.tunnel_ops, in->args.vc.user_id);
> +		{
> +			uint32_t *converted_idx;
> +			uint32_t converted_idx_num = 0;
> +
> +			converted_idx = calloc(in->args.vc.actions_n,
> +					       sizeof(*converted_idx));
> +			if (!converted_idx) {
> +				fprintf(stderr,
> +					"Warning: Failed to allocate conversion index list\n");
> +				break;
> +			}
> +
> +			/* Convert from action_prog_data to rte_flow_action_prog. */
> +			ret = convert_action_prog_to_rte_flow(in->args.vc.actions,
> +						      0,
> +						      converted_idx,
> +						      in->args.vc.actions_n,
> +						      &converted_idx_num);
> +			if (ret < 0)
> +				fprintf(stderr,
> +					"Warning: Failed to convert program action data: %s\n",
> +					strerror(-ret));
> +			port_flow_create(in->port, &in->args.vc.attr,
> +					 in->args.vc.pattern, in->args.vc.actions,
> +					 &in->args.vc.tunnel_ops, in->args.vc.user_id);

Does this code call port_flow_create() even when
convert_action_prog_to_rte_flow() returns an error? Should it skip the
call to port_flow_create() if conversion failed?

> +			free_action_prog_converted(in->args.vc.actions,
> +						   converted_idx,
> +						   converted_idx_num);
> +			free(converted_idx);
> +		}
>  		break;
>  	case DESTROY:
>  		port_flow_destroy(in->port, in->args.destroy.rule_n,