On 10/14/2020 7:16 AM, Andrew Rybchenko wrote:
On 10/13/20 7:12 PM, Ferruh Yigit wrote:
On 10/13/2020 4:39 PM, Andrew Rybchenko wrote:
On 10/13/20 6:32 PM, Ferruh Yigit wrote:
On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
Use ENODEV as the error code if specified port ID is invalid.
Signed-off-by: Andrew Rybchenko <arybche...@solarflare.com>
---
lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
lib/librte_ethdev/rte_ethdev.h | 46
+++++++++++++++++++++++-----------
2 files changed, 54 insertions(+), 36 deletions(-)
diff --git a/lib/librte_ethdev/rte_ethdev.c
b/lib/librte_ethdev/rte_ethdev.c
index 5b7979a3b8..1f862f918a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id,
char *name)
{
char *tmp;
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
Thanks Andrew, +1 to this error unification.
This will be API change without deprecation notice, cc'ed techboard
for it.
Yes, thanks.
If this should (almost) always return '-ENODEV', does it make sense to
make another wrapper macro for it, to prevent later other error types
used again.
Unlikely, since most likely the line will be simply copied.
RTE_ETH_VALID_PORTID_OR_ERR_RET will remain in any case, so
it will be possible to misuse it anyway.
Agree it won't prevent misuse completely but may help, anyway I don't
have a strong opinion here, if you think that is not needed, that is OK.
And there are a few instances returning '-1', are they left
intentionally?
Yes. Inside ethdev it is either socket_id or fd in these cases.
Can't those two also updated to return '-ENODEV' when 'port_id' is not
valid?
I think no.
1. rte_eth_dev_socket_id() should not return -ENODEV since it
can return -1 even if port ID is valid if fact (I see
printouts from time to time if I'm not mistaken) and
typically handled as unspecified NUMA node ID.
2. rte_eth_dev_rx_intr_ctl_q_get_fd() explicitly says that -1
is returned on error. The function is still experimental
and we can change it, but I'd say that -1 match typical
behavior for functions returning file descriptor.
Let's limit the changeset to switch from EINVAL to ENODEV.
OK.
It looks like there is no objection on the API part, so I will proceed with it
but will get as one of the last a few patches before -rc1 to prevent other
existing ethdev patches to conflict.
