[PATCH v6 03/27] net/intel/common: add common flow action parsing

[Burakov, Anatoly]

On 6/3/2026 3:03 PM, Bruce Richardson wrote:
> On Fri, May 29, 2026 at 04:36:05PM +0100, Anatoly Burakov wrote:
>> Currently, each driver has their own code for action parsing, which results
>> in a lot of duplication and subtle mismatches in behavior between drivers.
>>
>> Add common infrastructure, based on the following assumptions:
>>
>> - All drivers support at most 32 actions at once, but usually far less
>> - Not every action is supported by all drivers
>> - We can check a few common things to filter out obviously wrong actions
>> - Driver performs semantic checks on all valid actions
>>
>> So, the intention is to reject everything we can reasonably reject at the
>> outset without knowing anything about the drivers, parametrize what is
>> trivial to parametrize, and leave the rest for the driver to implement.
>>
>> While we're at it, also add logging infrastructure for Intel common code,
>> using the new component name defines that are automatically passed to each
>> DPDK driver as it is being built.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>> ---
>>   drivers/net/intel/common/flow_check.h | 279 ++++++++++++++++++++++++++
>>   drivers/net/intel/common/log.h        |  40 ++++
>>   2 files changed, 319 insertions(+)
>>   create mode 100644 drivers/net/intel/common/flow_check.h
>>   create mode 100644 drivers/net/intel/common/log.h
>>
> 
> <snip>
> 
>> +/**
>> + * Validate and parse a list of rte_flow_action into a parsed action list.
>> + *
>> + * @param actions pointer to array of rte_flow_action, terminated by RTE_FLOW_ACTION_TYPE_END
>> + * @param param pointer to ci_flow_actions_check_param structure (can be NULL)
>> + * @param parsed_actions pointer to ci_flow_actions structure to store parsed actions
>> + * @param error pointer to rte_flow_error structure for error reporting
>> + *
>> + * @return 0 on success, negative errno on failure.
>> + */
>> +static inline int
>> +ci_flow_check_actions(const struct rte_flow_action *actions,
>> +	const struct ci_flow_actions_check_param *param,
>> +	struct ci_flow_actions *parsed_actions,
>> +	struct rte_flow_error *error)
>> +{
>> +	size_t i = 0;
>> +	int ret;
>> +
>> +	if (actions == NULL) {
>> +		return rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION,
>> +				NULL, "Missing actions");
>> +	}
>> +
>> +	/* reset the list */
>> +	*parsed_actions = (struct ci_flow_actions){0};
>> +
>> +	while (actions[i].type != RTE_FLOW_ACTION_TYPE_END) {
>> +		const struct rte_flow_action *action = &actions[i++];
>> +
>> +		/* skip VOID actions */
>> +		if (action->type == RTE_FLOW_ACTION_TYPE_VOID)
>> +			continue;
>> +
>> +		/* generic validation for actions - this will check against param as well */
>> +		ret = __flow_action_check_generic(action, param, error);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* check against global maximum number of actions */
>> +		if (parsed_actions->count >= RTE_DIM(parsed_actions->actions)) {
>> +			return rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION,
>> +							action, "Too many actions");
>> +		}
>> +		/* user may have specified a maximum number of actions */
>> +		if (param != NULL && param->max_actions != 0 &&
>> +		    parsed_actions->count >= param->max_actions) {
>> +			return rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION,
>> +							action, "Too many actions");
>> +		}
>> +		/* add action to the list */
>> +		CI_DRV_LOG(DEBUG, "Parsed action %u: type=%s", parsed_actions->count,
>> +			   ci_flow_action_type_to_str(action->type));
>> +		parsed_actions->actions[parsed_actions->count++] = action;
>> +	}
>> +
>> +	/* now, call into user validation if specified */
>> +	if (param != NULL && param->check != NULL) {
>> +		ret = param->check(parsed_actions, param, error);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
> 
> Running an AI review on this code myself, it highlights the fact that we
> are missing a check for an empty parsed_actions array here, and not all
> check handlers verify the length as being >0 before dereferencing. For
> example, in patch 10, the callbacks don't explicitly check for empty lists
> I think it would be reasonable to have the return -EINVAL before this
> callback check, right?

Yep, that is correct. Empty list should fail *before* we go into use 
callbacks. It is safe in the sense that references are never invalid, 
and well-behaved callbacks should check count, but that only reasonably 
applies to callbacks expecting multiple actions, but not just one.

> 
>> +	/* if we didn't parse anything, valid action list is empty */
>> +	return parsed_actions->count == 0 ? -EINVAL : 0;
>> +}
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
> <snip>


-- 
Thanks,
Anatoly