Re: [PATCH v9 1/2] ethdev: add query and update sync and async function calls

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 2/2/23 14:54, Gregory Etelson wrote:
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 7d0c24366c..4554c8f021 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1883,3 +1883,85 @@ rte_flow_async_action_handle_query(uint16_t port_id,
>   					  action_handle, data, user_data, error);
>   	return flow_err(port_id, ret, error);
>   }
> +
> +int
> +rte_flow_action_handle_query_update(uint16_t port_id,
> +				    struct rte_flow_action_handle *handle,
> +				    const void *update, void *query,
> +				    enum rte_flow_query_update_mode mode,
> +				    struct rte_flow_error *error)
> +{
> +	int ret;
> +	struct rte_eth_dev *dev;
> +	const struct rte_flow_ops *ops;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	if (!handle)
> +		return -EINVAL;
> +	dev = &rte_eth_devices[port_id];
> +	ops = rte_flow_ops_get(port_id, error);
> +	if (!ops)
> +		return -ENOTSUP;
> +	if (update && query) {
> +		if (!ops->action_handle_query_update)
> +			return -ENOTSUP;
> +		if (mode != RTE_FLOW_QU_QUERY_FIRST &&
> +		    mode != RTE_FLOW_QU_UPDATE_FIRST)
> +			return -EINVAL;

Shouldn't it be checked in any case?
Also I'd initialize RTE_FLOW_QU_QUERY_FIRST to 1 on enum
definition to ensure that just 0 is not used as a mode value.
Also "Required if both *update* and *query* are not NULL." does
not make sense in the description since you have no way to skip
it.

> +		ret = ops->action_handle_query_update(dev, handle, update,
> +						      query, mode, error);
> +	} else if (!update && query) {
> +		ret = rte_flow_action_handle_query(port_id, handle, query,
> +						   error);
> +	} else if (update && !query) {
> +		ret = rte_flow_action_handle_update(port_id, handle, update,
> +						    error);
> +	} else {
> +		return -EINVAL;
> +	}

IMHO logic is wrong above since it should be sufficient to
implement just one action_handle_query_update callback instead
of all three.

I.e. if action_handle_query_update is available, it should be
used in any case.

> +	return flow_err(port_id, ret, error);
> +}
>   };
>   
>   INTERNAL {

same for async API