Hello Andrew,
> 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.
>
Fixed in v10.
> > + 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.
>
Fixed in v10.
> > + return flow_err(port_id, ret, error);
> > +}
> > };
> >
> > INTERNAL {
>
> same for async API
Fixed in v10.
