On 4/13/2021 9:58 AM, Thomas Monjalon wrote:
13/04/2021 10:44, Andrew Rybchenko:
On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
@@ -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?
I agree with all your comments Andrew.
+1
@@ -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.
Indeed it can be used in the data path of some algorithms.
