Hi, Thanks Andrew,
All has been fixed in v3, please review it, thanks.
在 2021/4/13 16:44, Andrew Rybchenko 写道:
On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
This patch adds more sanity checks in control path APIs.
Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
Fixes: 0366137722a0 ("ethdev: check for invalid device name")
Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process
model")
Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Fixes: f8244c6399d9 ("ethdev: increase port id range")
Cc: sta...@dpdk.org
Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Many thanks for working on it. Few notes below.
[snip]
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 6b5cfd6..e1655b5 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
{
int ret;
+ if (owner == NULL)
+ return -EINVAL;
+
Here and in many-many cases below I think the order of checks
is important in cases when different error codes are returned.
When there is no any very good reasons why arguments should
be checked in different order, arguments should be checked in
order specified in function prototype. In this cases (and many
cases below), port_id should be checked first.
In this particular case it means that the pointer check
should be done in a static helper function.
One more point is error logging in the case of failure.
Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
find out that some of messages should be made INFO or DEBUG.
Something like:
RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
port_id);
I'm not 100% sure in format, but my requirements are:
- log messages should be unique
- log messages should be human readable (i.e. I'd avoid
usage of function name)
- log messages should provide enough information to understand
what went wrong and provide context (basically it correlates
with uniqueness requirement)
@Thomas, @Ferruh, what do you think? It would be good if we
reach an argement before mass changes are done?
@@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t
queue_id, uint32_t free_cnt)
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
+ if (queue_id >= dev->data->nb_tx_queues) {
+ RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
+ dev->data->nb_tx_queues);
+ return -EINVAL;
+ }
+
I'm not 100% sure that it is a control path.
/* Call driver to free pending mbufs. */
ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
free_cnt);
[snip]
.
