[dpdk-dev] [PATCH v2] ethdev: add sanity checks in control APIs

Andrew Rybchenko andrew.rybchenko at oktetlabs.ru
Tue Apr 13 10:44:28 CEST 2021


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: stable at dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29 at 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]


More information about the dev mailing list