On 3/19/21 9:12 PM, Thomas Monjalon wrote:
15/03/2021 10:15, Thomas Monjalon:
15/03/2021 10:08, Andrew Rybchenko:
On 3/15/21 11:55 AM, Thomas Monjalon wrote:
15/03/2021 09:43, Andrew Rybchenko:
On 3/15/21 10:54 AM, Thomas Monjalon wrote:
15/03/2021 08:18, Andrew Rybchenko:
On 3/12/21 8:46 PM, Thomas Monjalon wrote:
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -255,18 +255,19 @@ rte_flow_ops_get(uint16_t port_id, struct rte_flow_error
*error)
if (unlikely(!rte_eth_dev_is_valid_port(port_id)))
code = ENODEV;
- else if (unlikely(!dev->dev_ops->filter_ctrl ||
- dev->dev_ops->filter_ctrl(dev,
- RTE_ETH_FILTER_GENERIC,
- RTE_ETH_FILTER_GET,
- &ops) ||
- !ops))
- code = ENOSYS;
+ else if (unlikely(dev->dev_ops->flow_ops_get == NULL))
+ code = ENOTSUP;
It is described as:
-ENOTSUP: valid but unsupported rule specification (e.g.
partial bit-masks are unsupported).
So, it looks different. May be it is really better to keep
ENOSYS.
else
- return ops;
- rte_flow_error_set(error, code, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
- NULL, rte_strerror(code));
- return NULL;
+ code = dev->dev_ops->flow_ops_get(dev, &ops);
+ if (code == 0 && ops == NULL)
+ code = EACCES;
It looks something new. I think it should be mentioned in flow_ops_get
type documentation (similar to eth_promiscuous_enable_t) and
rte_flow_validate() etc functions
return values description.
It is an internal function used only in rte_flow.c.
The real consequence is to set rte_errno in a lot of rte_flow API.
Not sure there is a good way to document the code details.
Other codes are not documented in rte_flow.h
First of all it is a behaviour of the flow_ops_get callback and
driver developers should know that it is a legal to return 0 and
ops==NULL and know what it means.
The combination code 0 and ops NULL is not new.
Previously, it was returning ENOSYS.
I've just given a more meaningful error code: EACCES,
while replacing ENOSYS with ENOTSUP for the other case.
Yes, exactly. What I'm trying to say that it would be
helpful to make it a bit more transparent to PMD developers.
Yes, it was not documented before, I agree. I think it is
a good time to improve documentation.
Second, it is visible as rte_flow_validate() (and other functions
which use rte_flow_ops_get()) return value value which has
special meaning. So, should be documented.
Yes, I should update the API doc where ENOSYS was mentioned.
Or probably better: I should keep the error code ENOSYS
and do not break API.
Preference?
Good question. I think we should not distinguish NULL callback
and NULL ops returned by not-NULL callback. So, I think
keeping ENOSYS is the best option here.
OK, thank you for the review.
So the conclusion is: keep ENOSYS and document NULL ops case.
After more thoughts, I don't think we need to insist on the NULL ops case.
The function rte_flow_ops_get returns the ops,
and it is documented that returning NULL is an error.
So the function is just setting ENOSYS error code to have
an error code associated with returning NULL.
For the PMD, returning an ops NULL has no interest.
May be I misunderstand above, but
One driver may support different NIC types. It could support flow API
for one NIC type and do not support for another. Sometimes it could
be easier to sort it out in flow ops get callback rather than
provide different set of ethdev ops callback. If so, returning NULL
ops makes sense and easier way to say that flow API is not supported
(vs, for example, providing dummy callbacks which return ENOSYS).
