Re: [PATCH v5 04/27] net/intel/common: add common flow attr validation

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 5/27/2026 3:28 PM, Bruce Richardson wrote:
> On Mon, May 25, 2026 at 03:06:23PM +0100, Anatoly Burakov wrote:
>> There are a lot of commonalities between what kinds of flow attr each Intel
>> driver supports. Add a helper function that will validate attr based on
>> common requirements and (optional) parameter checks.
>>
>> Things we check for:
>> - Rejecting NULL attr (obviously)
>> - Default to ingress flows
>> - Transfer, group, priority, and egress are not allowed unless requested
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   drivers/net/intel/common/flow_check.h | 69 +++++++++++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>
>> diff --git a/drivers/net/intel/common/flow_check.h b/drivers/net/intel/common/flow_check.h
>> index 74fb28ae3d..0572028664 100644
>> --- a/drivers/net/intel/common/flow_check.h
>> +++ b/drivers/net/intel/common/flow_check.h
>> @@ -54,6 +54,7 @@ ci_flow_action_type_in_list(const enum rte_flow_action_type type,
>>   /* Forward declarations */
>>   struct ci_flow_actions;
>>   struct ci_flow_actions_check_param;
>> +struct ci_flow_attr_check_param;
>>   
>>   static inline const char *
>>   ci_flow_action_type_to_str(enum rte_flow_action_type type)
>> @@ -271,6 +272,74 @@ ci_flow_check_actions(const struct rte_flow_action *actions,
>>   	return parsed_actions->count == 0 ? -EINVAL : 0;
>>   }
>>   
>> +/**
>> + * Parameter structure for attr check.
>> + */
>> +struct ci_flow_attr_check_param {
>> +	bool allow_priority; /**< True if priority attribute is allowed. */
>> +	bool allow_transfer; /**< True if transfer attribute is allowed. */
>> +	bool allow_group;    /**< True if group attribute is allowed. */
>> +	bool expect_egress;  /**< True if egress attribute is expected. */
>> +};
>> +
>> +/**
>> + * Validate rte_flow_attr structure against specified constraints.
>> + *
>> + * @param attr Pointer to rte_flow_attr structure to validate.
>> + * @param attr_param Pointer to ci_flow_attr_check_param structure specifying constraints.
>> + * @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_attr(const struct rte_flow_attr *attr,
>> +		const struct ci_flow_attr_check_param *attr_param,
>> +		struct rte_flow_error *error)
>> +{
>> +	if (attr == NULL) {
>> +		return rte_flow_error_set(error, EINVAL,
>> +					  RTE_FLOW_ERROR_TYPE_ATTR, attr,
>> +					  "NULL attribute");
>> +	}
>> +
>> +	/* Direction must be either ingress or egress */
>> +	if (attr->ingress == attr->egress) {
>> +		return rte_flow_error_set(error, EINVAL,
>> +					  RTE_FLOW_ERROR_TYPE_ATTR, attr,
>> +					  "Either ingress or egress must be set");
>> +	}
>> +
>> +	/* Expect ingress by default */
>> +	if (attr->egress && (attr_param == NULL || !attr_param->expect_egress)) {
>> +		return rte_flow_error_set(error, EINVAL,
>> +					  RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, attr,
>> +					  "Egress not supported");
>> +	}
> 
> I think "allow_egress" is possibly a better name here. "expect_egress"
> implies that egress must be present, but the logic here seems to be only
> allowing egress if the flag is provided.

No, the check right above this prevents unspecified egress *if* ingress 
is also specified. Meaning, one of egress/ingress *must* be specified, 
and if it's egress, it is only allowed when we are expecting egress.

-- 
Thanks,
Anatoly