[v4] ethdev: add sanity checks in control APIs

  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@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v4:
* add error logging.
* delete check in control path API.

v3:
* set port_id checked first.
* add error logging.

v2:
* Removed unnecessary checks.
* Deleted checks in internal API.
* Added documentation in the header file.
---
 lib/librte_ethdev/rte_ethdev.c | 288 ++++++++++++++++++++++++++++++++++++++---
 lib/librte_ethdev/rte_ethdev.h |  20 ++-
 2 files changed, 287 insertions(+), 21 deletions(-)
  

On 4/15/21 3:52 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@dpdk.org

Many thanks. I'll keep log messages gramma review to native
speekers. Content looks good to me.

One minor issue below lost on previous review.

Other than that,

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

[snip]

> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (dev_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];

I think it is better to keep:
    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
    dev = &rte_eth_devices[port_id];
together since they are tightly related. I.e. I'd even remove
empty line between them when it is present and add args
sanity check after the dev assignment.
     >
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);

In theory, the first argument is sufficient to make the ops
check, but I think it is the right solution to keep it as is
since current tendency is to check operation support when
driver callback is really required and we're going to use it.
However, if we do it just after port_id check, we'll have a
way to check for callback support without any side effects
if we provide invalid argument value. I.e. if -ENOTSUP is
returned, callback is not supported, if -EINVAL, callback is
supported (but argument is invalid and nothing done).
However, it looks a bit fragile and not always possible.
Thoughts on it are welcome.
  

在 2021/4/15 16:15, Andrew Rybchenko 写道:
> On 4/15/21 3:52 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@dpdk.org
> 
> Many thanks. I'll keep log messages gramma review to native
> speekers. Content looks good to me.
> 
> One minor issue below lost on previous review.
> 
> Other than that,
> 
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> [snip]
> 
>> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (dev_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
> 
> I think it is better to keep:
>      RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>      dev = &rte_eth_devices[port_id];
> together since they are tightly related. I.e. I'd even remove
> empty line between them when it is present and add args
> sanity check after the dev assignment.
>       >
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> 
Thanks Andrew, this has been fixed in v5.

> In theory, the first argument is sufficient to make the ops
> check, but I think it is the right solution to keep it as is
> since current tendency is to check operation support when
> driver callback is really required and we're going to use it.
> However, if we do it just after port_id check, we'll have a
> way to check for callback support without any side effects
> if we provide invalid argument value. I.e. if -ENOTSUP is
> returned, callback is not supported, if -EINVAL, callback is
> supported (but argument is invalid and nothing done).
> However, it looks a bit fragile and not always possible.
> Thoughts on it are welcome.
> .
>
  

15/04/2021 10:15, Andrew Rybchenko:
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> 
> In theory, the first argument is sufficient to make the ops
> check, but I think it is the right solution to keep it as is
> since current tendency is to check operation support when
> driver callback is really required and we're going to use it.
> However, if we do it just after port_id check, we'll have a
> way to check for callback support without any side effects
> if we provide invalid argument value. I.e. if -ENOTSUP is
> returned, callback is not supported, if -EINVAL, callback is
> supported (but argument is invalid and nothing done).
> However, it looks a bit fragile and not always possible.
> Thoughts on it are welcome.

Sorry I don't understand it fully.
You say we should check for ENOTSUP at the very beginning?
  

On 4/15/21 2:57 PM, Thomas Monjalon wrote:
> 15/04/2021 10:15, Andrew Rybchenko:
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>> In theory, the first argument is sufficient to make the ops
>> check, but I think it is the right solution to keep it as is
>> since current tendency is to check operation support when
>> driver callback is really required and we're going to use it.
>> However, if we do it just after port_id check, we'll have a
>> way to check for callback support without any side effects
>> if we provide invalid argument value. I.e. if -ENOTSUP is
>> returned, callback is not supported, if -EINVAL, callback is
>> supported (but argument is invalid and nothing done).
>> However, it looks a bit fragile and not always possible.
>> Thoughts on it are welcome.
> Sorry I don't understand it fully.
> You say we should check for ENOTSUP at the very beginning?

I'm just trying to consider it and understand if it would be
right or wrong.
  

On 15/04/2021 01:52, Min Hu (Connor) wrote:
> This patch adds more sanity checks in control path APIs.
> 

Hi Connor,

A few general comments,

--
Some of the functions have unit tests, you could consider adding unit
tests for the new checks. Considering the checks are not subtle and
unlikely to be messed up in future, not adding unit tests is not a
blocker imho.

--
It took me a while to get what you meant with "by NULL". It's usage
seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
better way to phrase this generically, but I think it is more useful to
say what is NULL.

e.g. "Failed to convert NULL to string\n" is very generic and would be
better as "Failed to convert NULL link to string\n" . ok, still a bit
generic but more of a clue.

I won't comment on each log message individually but I've added a few
suggestions here and there.
--

Did you check the usage of these functions in DPDK, and if the return
value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
iterator functions. I'm not sure that having a return check is needed in
that case, but there could be other cases where you want to take some
different action now.

some other comments inlined,

> 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@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v4:
> * add error logging.
> * delete check in control path API.
> 
> v3:
> * set port_id checked first.
> * add error logging.
> 
> v2:
> * Removed unnecessary checks.
> * Deleted checks in internal API.
> * Added documentation in the header file.
> ---
>  lib/librte_ethdev/rte_ethdev.c | 288 ++++++++++++++++++++++++++++++++++++++---
>  lib/librte_ethdev/rte_ethdev.h |  20 ++-
>  2 files changed, 287 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 6b5cfd6..e734a30 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -199,6 +199,16 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>  	char *cls_str = NULL;
>  	int str_size;
>  
> +	if (iter == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");

"Failed to init iterator for NULL iterator\n"

> +		return -EINVAL;
> +	}
> +
> +	if (devargs_str == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");

"Failed to init iterator for NULL devargs\n"

> +		return -EINVAL;
> +	}
> +
>  	memset(iter, 0, sizeof(*iter));
>  
>  	/*
> @@ -293,6 +303,11 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>  uint16_t
>  rte_eth_iterator_next(struct rte_dev_iterator *iter)
>  {
> +	if (iter == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate next for NULL\n");
> +		return RTE_MAX_ETHPORTS;
> +	}
> +
>  	if (iter->cls == NULL) /* invalid ethdev iterator */
>  		return RTE_MAX_ETHPORTS;
>  
> @@ -322,6 +337,11 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
>  void
>  rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
>  {
> +	if (iter == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator clear up for NULL\n");
> +		return;
> +	}
> +
>  	if (iter->bus_str == NULL)
>  		return; /* nothing to free in pure class filter */
>  	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
> @@ -622,6 +642,11 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
>  int
>  rte_eth_dev_owner_new(uint64_t *owner_id)
>  {
> +	if (owner_id == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get owner id by NULL\n");
> +		return -EINVAL;
> +	}
> +
>  	eth_dev_shared_data_prepare();
>  
>  	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
> @@ -645,6 +670,12 @@ eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
>  		return -ENODEV;
>  	}
>  
> +	if (new_owner == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	if (!eth_is_valid_owner_id(new_owner->id) &&
>  	    !eth_is_valid_owner_id(old_owner_id)) {
>  		RTE_ETHDEV_LOG(ERR,
> @@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
>  int
>  rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
>  {
> -	int ret = 0;
> -	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
> -
> -	eth_dev_shared_data_prepare();
> +	struct rte_eth_dev *ethdev;
>  
> -	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
>  
> -	if (port_id >= RTE_MAX_ETHPORTS || !eth_dev_is_allocated(ethdev)) {
> +	ethdev = &rte_eth_devices[port_id];
> +	if (!eth_dev_is_allocated(ethdev)) {
>  		RTE_ETHDEV_LOG(ERR, "Port id %"PRIu16" is not allocated\n",
>  			port_id);
> -		ret = -ENODEV;
> -	} else {
> -		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
> +		return -ENODEV;
>  	}
>  
> +	if (owner == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}

This fn uses both %u and %"PRIu16" for port_id

> +
> +	eth_dev_shared_data_prepare();
> +
> +	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
> +	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
>  	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  int
> @@ -820,7 +858,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>  {
>  	uint16_t pid;
>  
> -	if (name == NULL) {
> +	if (name == NULL || port_id == NULL) {

Most of the other new checks have individual checks for NULL ptrs, this
one doesn't. I'm not sure if it needs individual checks, but it is
better to be consistent.

>  		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>  		return -EINVAL;
>  	}
> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (dev_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> @@ -2139,6 +2183,12 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	if (rx_queue_id >= dev->data->nb_rx_queues) {
>  		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
> @@ -2310,6 +2360,13 @@ rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Tx hairpin queue to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	if (tx_queue_id >= dev->data->nb_tx_queues) {
>  		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
> @@ -2459,6 +2516,11 @@ int
>  rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>  		buffer_tx_error_fn cbfn, void *userdata)
>  {
> +	if (buffer == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set error callback for NULL\n");
> +		return -EINVAL;
> +	}
> +
>  	buffer->error_callback = cbfn;
>  	buffer->error_userdata = userdata;
>  	return 0;
> @@ -2607,6 +2669,13 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (eth_link == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u link by NULL\n",

"by NULL" does not read nicely to me. How about
"Failed to get ethdev port %u for NULL link"

> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	if (dev->data->dev_conf.intr_conf.lsc &&
> @@ -2627,6 +2696,13 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (eth_link == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u nowait link by NULL\n",
> +			port_id);

Same comment as for previous function, except you'll probably want to
keep nowait somewhere to distinguish.
"Failed to get nowait ethdev port %u for NULL link" ?

> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	if (dev->data->dev_conf.intr_conf.lsc &&
> @@ -2667,6 +2743,16 @@ rte_eth_link_speed_to_str(uint32_t link_speed)
>  int
>  rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link)
>  {

The messages are very generic, especially the second one. Suggestions below

> +	if (str == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");

"Failed to convert link to NULL string\n"

> +		return -EINVAL;
> +	}

You should probably also check the 'len' param for str here also.

> +
> +	if (eth_link == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");

"Failed to convert NULL link to string\n"

> +		return -EINVAL;
> +	}
> +
>  	if (eth_link->link_status == ETH_LINK_DOWN)
>  		return snprintf(str, len, "Link down");
>  	else
> @@ -2685,6 +2771,12 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (stats == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u stats by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	memset(stats, 0, sizeof(*stats));
>  
> @@ -3258,6 +3350,13 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (fw_version == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u fw version by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}

Some of the drivers already check for NULL ptrs and some don't. It means
some checks could be removed from the drivers, but it doesn't seem a
worthwhile effort unless they are needing some rework anyway.

'fw_size == 0' could also be checked here

> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
> @@ -3278,6 +3377,14 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  	};
>  	int diag;
>  
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (dev_info == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u info by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Init dev_info before port_id check since caller does not have
>  	 * return status and does not know if get is successful or not.
> @@ -3285,7 +3392,6 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>  	dev_info->switch_info.domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>  
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);

This is a slight change in behaviour. With an invalid port, previously
dev_info was memset to 0. Does it need to be? I guess no.

At the very least the comment block is incorrect and should be changed.

>  	dev = &rte_eth_devices[port_id];
>  
>  	dev_info->rx_desc_lim = lim;
> @@ -3326,6 +3432,13 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>  	const uint32_t *all_ptypes;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (ptypes == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u supported ptypes by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +

'num == 0' could be checked here too.

>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
>  	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
> @@ -3435,6 +3548,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (mac_addr == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MAC address by NULL\n",

"by NULL" doesn't read correctly.
"Failed to get ethdev port %u MAC address for NULL param\n"

> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
>  
> @@ -3448,6 +3568,12 @@ rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (mtu == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MTU by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	*mtu = dev->data->mtu;
>  	return 0;
> @@ -3696,6 +3822,13 @@ rte_eth_dev_flow_ctrl_get(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (fc_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u flow conf by NULL\n",

I would at least add "flow *ctrl* conf" e.g.
"Failed to get ethdev port %u flow ctrl conf for NULL param\n"

> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->flow_ctrl_get, -ENOTSUP);
>  	memset(fc_conf, 0, sizeof(*fc_conf));
> @@ -3708,6 +3841,13 @@ rte_eth_dev_flow_ctrl_set(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (fc_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u flow conf to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	if ((fc_conf->send_xon != 0) && (fc_conf->send_xon != 1)) {
>  		RTE_ETHDEV_LOG(ERR, "Invalid send_xon, only 0/1 allowed\n");
>  		return -EINVAL;
> @@ -3725,6 +3865,13 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (pfc_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u priority flow conf to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	if (pfc_conf->priority > (ETH_DCB_NUM_USER_PRIORITIES - 1)) {
>  		RTE_ETHDEV_LOG(ERR, "Invalid priority, only 0-7 allowed\n");
>  		return -EINVAL;
> @@ -3744,9 +3891,6 @@ eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
>  {
>  	uint16_t i, num;
>  
> -	if (!reta_conf)
> -		return -EINVAL;
> -
>  	num = (reta_size + RTE_RETA_GROUP_SIZE - 1) / RTE_RETA_GROUP_SIZE;
>  	for (i = 0; i < num; i++) {
>  		if (reta_conf[i].mask)
> @@ -3763,9 +3907,6 @@ eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
>  {
>  	uint16_t i, idx, shift;
>  
> -	if (!reta_conf)
> -		return -EINVAL;
> -
>  	if (max_rxq == 0) {
>  		RTE_ETHDEV_LOG(ERR, "No receive queue is available\n");
>  		return -EINVAL;
> @@ -3796,6 +3937,13 @@ rte_eth_dev_rss_reta_update(uint16_t port_id,
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (reta_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss reta to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +

You may want to check reta_size as well in these rss functions?

>  	/* Check mask bits */
>  	ret = eth_check_reta_mask(reta_conf, reta_size);
>  	if (ret < 0)
> @@ -3824,6 +3972,12 @@ rte_eth_dev_rss_reta_query(uint16_t port_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (reta_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to query ethdev port %u rss reta by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	/* Check mask bits */
>  	ret = eth_check_reta_mask(reta_conf, reta_size);
>  	if (ret < 0)
> @@ -3845,6 +3999,12 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (rss_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss hash to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>  	if (ret != 0)
>  		return ret;
> @@ -3872,6 +4032,13 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (rss_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u rss hash conf by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
>  	return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
> @@ -4027,6 +4194,13 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (addr == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to add ethdev port %u MAC address to NULL\n",
> +			port_id);

This message is a bit confusing. Perhaps,
"Failed to add NULL MAC address to ethdev port %u\n"

thanks,
Kevin.

> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>  
> @@ -4077,6 +4251,13 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>  	int index;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (addr == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to remove ethdev port %u MAC address by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>  
> @@ -4109,6 +4290,12 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (addr == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u default MAC address to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	if (!rte_is_valid_assigned_ether_addr(addr))
>  		return -EINVAL;
>  
> @@ -4164,6 +4351,12 @@ rte_eth_dev_uc_hash_table_set(uint16_t port_id, struct rte_ether_addr *addr,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (addr == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u uc hash table to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	if (rte_is_zero_ether_addr(addr)) {
>  		RTE_ETHDEV_LOG(ERR, "Port %u: Cannot add NULL MAC address\n",
> @@ -4265,6 +4458,13 @@ rte_eth_mirror_rule_set(uint16_t port_id,
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (mirror_conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u mirror rule to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	if (mirror_conf->rule_type == 0) {
>  		RTE_ETHDEV_LOG(ERR, "Mirror rule type can not be 0\n");
>  		return -EINVAL;
> @@ -5208,6 +5408,13 @@ rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (timestamp == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Rx timestamp by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp, -ENOTSUP);
> @@ -5222,6 +5429,13 @@ rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (timestamp == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Tx timestamp by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_tx_timestamp, -ENOTSUP);
> @@ -5248,6 +5462,13 @@ rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (timestamp == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u time by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_time, -ENOTSUP);
> @@ -5261,6 +5482,13 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (timestamp == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to write ethdev port %u time to NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_write_time, -ENOTSUP);
> @@ -5274,6 +5502,13 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>  	struct rte_eth_dev *dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (clock == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u clock by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -ENOTSUP);
> @@ -5372,6 +5607,12 @@ rte_eth_dev_get_dcb_info(uint16_t port_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (dcb_info == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u dcb info by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	memset(dcb_info, 0, sizeof(struct rte_eth_dcb_info));
>  
> @@ -5423,6 +5664,12 @@ rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> +	if (cap == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u hairpin capability by NULL\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get, -ENOTSUP);
>  	memset(cap, 0, sizeof(*cap));
> @@ -5629,6 +5876,11 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  	struct rte_eth_representor_info *info = NULL;
>  	size_t size;
>  
> +	if (ethdev == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Failed to get device representor info from NULL\n");
> +		return -EINVAL;
> +	}
> +
>  	if (type == RTE_ETH_REPRESENTOR_NONE)
>  		return 0;
>  	if (repr_id == NULL)
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 3b773b6..c1e5d4b 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2702,6 +2702,7 @@ int rte_eth_allmulticast_get(uint16_t port_id);
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>  
> @@ -2717,6 +2718,7 @@ int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
>  
> @@ -2752,7 +2754,7 @@ const char *rte_eth_link_speed_to_str(uint32_t link_speed);
>   * @param eth_link
>   *   Link status returned by rte_eth_link_get function
>   * @return
> - *   Number of bytes written to str array.
> + *   Number of bytes written to str array or -EINVAL if bad parameter.
>   */
>  __rte_experimental
>  int rte_eth_link_to_str(char *str, size_t len,
> @@ -2997,6 +2999,7 @@ int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>   * @return
>   *   - (0) if successful
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>  
> @@ -3041,6 +3044,7 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>  
> @@ -3060,6 +3064,7 @@ int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>   *   - (-ENOTSUP) if operation is not supported.
>   *   - (-ENODEV) if *port_id* invalid.
>   *   - (-EIO) if device is removed.
> + *   - (-EINVAL) if bad parameter.
>   *   - (>0) if *fw_size* is not enough to store firmware version, return
>   *          the size of the non truncated string.
>   */
> @@ -3103,6 +3108,7 @@ int rte_eth_dev_fw_version_get(uint16_t port_id,
>   *           only num entries will be filled into the ptypes array, but the full
>   *           count of supported ptypes will be returned.
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>  				     uint32_t *ptypes, int num);
> @@ -3153,6 +3159,7 @@ int rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
>   * @return
>   *   - (0) if successful.
>   *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu);
>  
> @@ -3347,7 +3354,7 @@ rte_eth_tx_buffer_init(struct rte_eth_dev_tx_buffer *buffer, uint16_t size);
>   * @param userdata
>   *   Arbitrary parameter to be passed to the callback function
>   * @return
> - *   0 on success, or -1 on error with rte_errno set appropriately
> + *   0 on success, or -EINVAL if bad parameter
>   */
>  int
>  rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
> @@ -3774,6 +3781,7 @@ int rte_eth_fec_set(uint16_t port_id, uint32_t fec_capa);
>   *   - (-ENOTSUP) if hardware doesn't support flow control.
>   *   - (-ENODEV)  if *port_id* invalid.
>   *   - (-EIO)  if device is removed.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_dev_flow_ctrl_get(uint16_t port_id,
>  			      struct rte_eth_fc_conf *fc_conf);
> @@ -3845,7 +3853,8 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *mac_addr,
>   *   - (0) if successful, or *mac_addr* didn't exist.
>   *   - (-ENOTSUP) if hardware doesn't support.
>   *   - (-ENODEV) if *port* invalid.
> - *   - (-EADDRINUSE) if attempting to remove the default MAC address
> + *   - (-EADDRINUSE) if attempting to remove the default MAC address.
> + *   - (-EINVAL) if MAC address is invalid.
>   */
>  int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>  				struct rte_ether_addr *mac_addr);
> @@ -4044,6 +4053,7 @@ int rte_eth_dev_rss_hash_update(uint16_t port_id,
>   *   - (-ENODEV) if port identifier is invalid.
>   *   - (-EIO) if device is removed.
>   *   - (-ENOTSUP) if hardware doesn't support RSS.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int
>  rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
> @@ -4112,6 +4122,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
>   *   - (-ENODEV) if port identifier is invalid.
>   *   - (-EIO) if device is removed.
>   *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-EINVAL) if bad parameter.
>   */
>  int rte_eth_dev_get_dcb_info(uint16_t port_id,
>  			     struct rte_eth_dcb_info *dcb_info);
> @@ -4628,6 +4639,7 @@ int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
>   *
>   * @return
>   *   - 0: Success.
> + *   - -EINVAL: Bad parameter.
>   */
>  int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
>  
> @@ -4694,6 +4706,7 @@ int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
>   *   - 0: Success.
>   *   - -ENODEV: The port ID is invalid.
>   *   - -ENOTSUP: The function is not supported by the Ethernet driver.
> + *   - -EINVAL: if bad parameter.
>   */
>  __rte_experimental
>  int
> @@ -4797,6 +4810,7 @@ rte_eth_dev_get_sec_ctx(uint16_t port_id);
>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-EINVAL) if bad parameter.
>   */
>  __rte_experimental
>  int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>
  

15/04/2021 14:04, Kevin Traynor:
> On 15/04/2021 01:52, Min Hu (Connor) wrote:
> > +	if (iter == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");
> 
> "Failed to init iterator for NULL iterator\n"

The word "Failed" looks weird in these checks.
What about "Cannot"?
Example: "Cannot init NULL iterator"

> > +	if (devargs_str == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");
> 
> "Failed to init iterator for NULL devargs\n"

"Cannot init iterator for NULL devargs"

> > +	if (owner == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
> > +			port_id);
> > +		return -EINVAL;
> > +	}
> 
> This fn uses both %u and %"PRIu16" for port_id

I don't see the benefit of PRIu16.

> > +	if (str == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");
> 
> "Failed to convert link to NULL string\n"

"Cannot convert link in NULL string"

> > +	if (eth_link == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");
> 
> "Failed to convert NULL link to string\n"

"Cannot convert NULL link"
  

15/04/2021 14:03, Andrew Rybchenko:
> On 4/15/21 2:57 PM, Thomas Monjalon wrote:
> > 15/04/2021 10:15, Andrew Rybchenko:
> >>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> >> In theory, the first argument is sufficient to make the ops
> >> check, but I think it is the right solution to keep it as is
> >> since current tendency is to check operation support when
> >> driver callback is really required and we're going to use it.
> >> However, if we do it just after port_id check, we'll have a
> >> way to check for callback support without any side effects
> >> if we provide invalid argument value. I.e. if -ENOTSUP is
> >> returned, callback is not supported, if -EINVAL, callback is
> >> supported (but argument is invalid and nothing done).
> >> However, it looks a bit fragile and not always possible.
> >> Thoughts on it are welcome.
> > Sorry I don't understand it fully.
> > You say we should check for ENOTSUP at the very beginning?
> 
> I'm just trying to consider it and understand if it would be
> right or wrong.

I think it's better to check things when they are required.
If the application does not give the right parameter,
it won't be caught until a supporting device will be used.

If the appplication gives the wrong parameter on purpose,
because it won't be supported, then it's better not calling the function.
  

On 4/15/21 3:20 PM, Thomas Monjalon wrote:
> 15/04/2021 14:03, Andrew Rybchenko:
>> On 4/15/21 2:57 PM, Thomas Monjalon wrote:
>>> 15/04/2021 10:15, Andrew Rybchenko:
>>>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>>>> In theory, the first argument is sufficient to make the ops
>>>> check, but I think it is the right solution to keep it as is
>>>> since current tendency is to check operation support when
>>>> driver callback is really required and we're going to use it.
>>>> However, if we do it just after port_id check, we'll have a
>>>> way to check for callback support without any side effects
>>>> if we provide invalid argument value. I.e. if -ENOTSUP is
>>>> returned, callback is not supported, if -EINVAL, callback is
>>>> supported (but argument is invalid and nothing done).
>>>> However, it looks a bit fragile and not always possible.
>>>> Thoughts on it are welcome.
>>> Sorry I don't understand it fully.
>>> You say we should check for ENOTSUP at the very beginning?
>>
>> I'm just trying to consider it and understand if it would be
>> right or wrong.
> 
> I think it's better to check things when they are required.
> If the application does not give the right parameter,
> it won't be caught until a supporting device will be used.
> 
> If the appplication gives the wrong parameter on purpose,
> because it won't be supported, then it's better not calling the function.
> 

OK, makes sense.
  

Thanks Kevin,
	all is fixed in v6, please review it, thanks.
	Some comments are below.

在 2021/4/15 20:04, Kevin Traynor 写道:
> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>> This patch adds more sanity checks in control path APIs.
>>
> 
> Hi Connor,
> 
> A few general comments,
> 
> --
> Some of the functions have unit tests, you could consider adding unit
> tests for the new checks. Considering the checks are not subtle and
> unlikely to be messed up in future, not adding unit tests is not a
> blocker imho.
> 
> --
> It took me a while to get what you meant with "by NULL". It's usage
> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
> better way to phrase this generically, but I think it is more useful to
> say what is NULL.
> 
> e.g. "Failed to convert NULL to string\n" is very generic and would be
> better as "Failed to convert NULL link to string\n" . ok, still a bit
> generic but more of a clue.
> 
> I won't comment on each log message individually but I've added a few
> suggestions here and there.
> --
> 
> Did you check the usage of these functions in DPDK, and if the return
> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
> iterator functions. I'm not sure that having a return check is needed in
> that case, but there could be other cases where you want to take some
> different action now.
> 
As iterator functions are all APIs, they may be used by APP directly.
I think param check is necessary.
> some other comments inlined,
> 
>> 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@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> v4:
>> * add error logging.
>> * delete check in control path API.
>>
>> v3:
>> * set port_id checked first.
>> * add error logging.
>>
>> v2:
>> * Removed unnecessary checks.
>> * Deleted checks in internal API.
>> * Added documentation in the header file.
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 288 ++++++++++++++++++++++++++++++++++++++---
>>   lib/librte_ethdev/rte_ethdev.h |  20 ++-
>>   2 files changed, 287 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 6b5cfd6..e734a30 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -199,6 +199,16 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>>   	char *cls_str = NULL;
>>   	int str_size;
>>   
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");
> 
> "Failed to init iterator for NULL iterator\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (devargs_str == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");
> 
> "Failed to init iterator for NULL devargs\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	memset(iter, 0, sizeof(*iter));
>>   
>>   	/*
>> @@ -293,6 +303,11 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>>   uint16_t
>>   rte_eth_iterator_next(struct rte_dev_iterator *iter)
>>   {
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate next for NULL\n");
>> +		return RTE_MAX_ETHPORTS;
>> +	}
>> +
>>   	if (iter->cls == NULL) /* invalid ethdev iterator */
>>   		return RTE_MAX_ETHPORTS;
>>   
>> @@ -322,6 +337,11 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
>>   void
>>   rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
>>   {
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator clear up for NULL\n");
>> +		return;
>> +	}
>> +
>>   	if (iter->bus_str == NULL)
>>   		return; /* nothing to free in pure class filter */
>>   	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
>> @@ -622,6 +642,11 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
>>   int
>>   rte_eth_dev_owner_new(uint64_t *owner_id)
>>   {
>> +	if (owner_id == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get owner id by NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	eth_dev_shared_data_prepare();
>>   
>>   	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> @@ -645,6 +670,12 @@ eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
>>   		return -ENODEV;
>>   	}
>>   
>> +	if (new_owner == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (!eth_is_valid_owner_id(new_owner->id) &&
>>   	    !eth_is_valid_owner_id(old_owner_id)) {
>>   		RTE_ETHDEV_LOG(ERR,
>> @@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
>>   int
>>   rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
>>   {
>> -	int ret = 0;
>> -	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
>> -
>> -	eth_dev_shared_data_prepare();
>> +	struct rte_eth_dev *ethdev;
>>   
>> -	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
>>   
>> -	if (port_id >= RTE_MAX_ETHPORTS || !eth_dev_is_allocated(ethdev)) {
>> +	ethdev = &rte_eth_devices[port_id];
>> +	if (!eth_dev_is_allocated(ethdev)) {
>>   		RTE_ETHDEV_LOG(ERR, "Port id %"PRIu16" is not allocated\n",
>>   			port_id);
>> -		ret = -ENODEV;
>> -	} else {
>> -		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
>> +		return -ENODEV;
>>   	}
>>   
>> +	if (owner == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
> 
> This fn uses both %u and %"PRIu16" for port_id
> 
>> +
>> +	eth_dev_shared_data_prepare();
>> +
>> +	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> +	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
>>   	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
>> -	return ret;
>> +
>> +	return 0;
>>   }
>>   
>>   int
>> @@ -820,7 +858,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>>   {
>>   	uint16_t pid;
>>   
>> -	if (name == NULL) {
>> +	if (name == NULL || port_id == NULL) {
> 
> Most of the other new checks have individual checks for NULL ptrs, this
> one doesn't. I'm not sure if it needs individual checks, but it is
> better to be consistent.
> 
>>   		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>>   		return -EINVAL;
>>   	}
>> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (dev_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>> @@ -2139,6 +2183,12 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (rx_queue_id >= dev->data->nb_rx_queues) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
>> @@ -2310,6 +2360,13 @@ rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Tx hairpin queue to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (tx_queue_id >= dev->data->nb_tx_queues) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
>> @@ -2459,6 +2516,11 @@ int
>>   rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>>   		buffer_tx_error_fn cbfn, void *userdata)
>>   {
>> +	if (buffer == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set error callback for NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	buffer->error_callback = cbfn;
>>   	buffer->error_userdata = userdata;
>>   	return 0;
>> @@ -2607,6 +2669,13 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u link by NULL\n",
> 
> "by NULL" does not read nicely to me. How about
> "Failed to get ethdev port %u for NULL link"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2627,6 +2696,13 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u nowait link by NULL\n",
>> +			port_id);
> 
> Same comment as for previous function, except you'll probably want to
> keep nowait somewhere to distinguish.
> "Failed to get nowait ethdev port %u for NULL link" ?
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2667,6 +2743,16 @@ rte_eth_link_speed_to_str(uint32_t link_speed)
>>   int
>>   rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link)
>>   {
> 
> The messages are very generic, especially the second one. Suggestions below
> 
>> +	if (str == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");
> 
> "Failed to convert link to NULL string\n"
> 
>> +		return -EINVAL;
>> +	}
> 
> You should probably also check the 'len' param for str here also.
> 
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");
> 
> "Failed to convert NULL link to string\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (eth_link->link_status == ETH_LINK_DOWN)
>>   		return snprintf(str, len, "Link down");
>>   	else
>> @@ -2685,6 +2771,12 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (stats == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u stats by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	memset(stats, 0, sizeof(*stats));
>>   
>> @@ -3258,6 +3350,13 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fw_version == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u fw version by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
> 
> Some of the drivers already check for NULL ptrs and some don't. It means
> some checks could be removed from the drivers, but it doesn't seem a
> worthwhile effort unless they are needing some rework anyway.
Agreed, this may be fixed in next patch.
> 
> 'fw_size == 0' could also be checked here
> 
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
>> @@ -3278,6 +3377,14 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   	};
>>   	int diag;
>>   
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (dev_info == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u info by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	/*
>>   	 * Init dev_info before port_id check since caller does not have
>>   	 * return status and does not know if get is successful or not.
>> @@ -3285,7 +3392,6 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>>   	dev_info->switch_info.domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>>   
>> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> This is a slight change in behaviour. With an invalid port, previously
> dev_info was memset to 0. Does it need to be? I guess no.
> 
> At the very least the comment block is incorrect and should be changed.
> 
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	dev_info->rx_desc_lim = lim;
>> @@ -3326,6 +3432,13 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>   	const uint32_t *all_ptypes;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (ptypes == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u supported ptypes by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
> 
> 'num == 0' could be checked here too.
> 
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
>>   	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
>> @@ -3435,6 +3548,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (mac_addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MAC address by NULL\n",
> 
> "by NULL" doesn't read correctly.
> "Failed to get ethdev port %u MAC address for NULL param\n"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
>>   
>> @@ -3448,6 +3568,12 @@ rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (mtu == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MTU by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	*mtu = dev->data->mtu;
>>   	return 0;
>> @@ -3696,6 +3822,13 @@ rte_eth_dev_flow_ctrl_get(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u flow conf by NULL\n",
> 
> I would at least add "flow *ctrl* conf" e.g.
> "Failed to get ethdev port %u flow ctrl conf for NULL param\n"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->flow_ctrl_get, -ENOTSUP);
>>   	memset(fc_conf, 0, sizeof(*fc_conf));
>> @@ -3708,6 +3841,13 @@ rte_eth_dev_flow_ctrl_set(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u flow conf to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if ((fc_conf->send_xon != 0) && (fc_conf->send_xon != 1)) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid send_xon, only 0/1 allowed\n");
>>   		return -EINVAL;
>> @@ -3725,6 +3865,13 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (pfc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u priority flow conf to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (pfc_conf->priority > (ETH_DCB_NUM_USER_PRIORITIES - 1)) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid priority, only 0-7 allowed\n");
>>   		return -EINVAL;
>> @@ -3744,9 +3891,6 @@ eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
>>   {
>>   	uint16_t i, num;
>>   
>> -	if (!reta_conf)
>> -		return -EINVAL;
>> -
>>   	num = (reta_size + RTE_RETA_GROUP_SIZE - 1) / RTE_RETA_GROUP_SIZE;
>>   	for (i = 0; i < num; i++) {
>>   		if (reta_conf[i].mask)
>> @@ -3763,9 +3907,6 @@ eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
>>   {
>>   	uint16_t i, idx, shift;
>>   
>> -	if (!reta_conf)
>> -		return -EINVAL;
>> -
>>   	if (max_rxq == 0) {
>>   		RTE_ETHDEV_LOG(ERR, "No receive queue is available\n");
>>   		return -EINVAL;
>> @@ -3796,6 +3937,13 @@ rte_eth_dev_rss_reta_update(uint16_t port_id,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (reta_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss reta to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
> 
> You may want to check reta_size as well in these rss functions?
> 
>>   	/* Check mask bits */
>>   	ret = eth_check_reta_mask(reta_conf, reta_size);
>>   	if (ret < 0)
>> @@ -3824,6 +3972,12 @@ rte_eth_dev_rss_reta_query(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (reta_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to query ethdev port %u rss reta by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	/* Check mask bits */
>>   	ret = eth_check_reta_mask(reta_conf, reta_size);
>>   	if (ret < 0)
>> @@ -3845,6 +3999,12 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (rss_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss hash to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>   	if (ret != 0)
>>   		return ret;
>> @@ -3872,6 +4032,13 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (rss_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u rss hash conf by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
>>   	return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>> @@ -4027,6 +4194,13 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to add ethdev port %u MAC address to NULL\n",
>> +			port_id);
> 
> This message is a bit confusing. Perhaps,
> "Failed to add NULL MAC address to ethdev port %u\n"
> 
> thanks,
> Kevin.
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>>   
>> @@ -4077,6 +4251,13 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>>   	int index;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to remove ethdev port %u MAC address by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>>   
>> @@ -4109,6 +4290,12 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u default MAC address to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (!rte_is_valid_assigned_ether_addr(addr))
>>   		return -EINVAL;
>>   
>> @@ -4164,6 +4351,12 @@ rte_eth_dev_uc_hash_table_set(uint16_t port_id, struct rte_ether_addr *addr,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u uc hash table to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (rte_is_zero_ether_addr(addr)) {
>>   		RTE_ETHDEV_LOG(ERR, "Port %u: Cannot add NULL MAC address\n",
>> @@ -4265,6 +4458,13 @@ rte_eth_mirror_rule_set(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (mirror_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u mirror rule to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (mirror_conf->rule_type == 0) {
>>   		RTE_ETHDEV_LOG(ERR, "Mirror rule type can not be 0\n");
>>   		return -EINVAL;
>> @@ -5208,6 +5408,13 @@ rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Rx timestamp by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp, -ENOTSUP);
>> @@ -5222,6 +5429,13 @@ rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Tx timestamp by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_tx_timestamp, -ENOTSUP);
>> @@ -5248,6 +5462,13 @@ rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u time by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_time, -ENOTSUP);
>> @@ -5261,6 +5482,13 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to write ethdev port %u time to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_write_time, -ENOTSUP);
>> @@ -5274,6 +5502,13 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (clock == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u clock by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -ENOTSUP);
>> @@ -5372,6 +5607,12 @@ rte_eth_dev_get_dcb_info(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (dcb_info == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u dcb info by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	memset(dcb_info, 0, sizeof(struct rte_eth_dcb_info));
>>   
>> @@ -5423,6 +5664,12 @@ rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (cap == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u hairpin capability by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get, -ENOTSUP);
>>   	memset(cap, 0, sizeof(*cap));
>> @@ -5629,6 +5876,11 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>>   	struct rte_eth_representor_info *info = NULL;
>>   	size_t size;
>>   
>> +	if (ethdev == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get device representor info from NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (type == RTE_ETH_REPRESENTOR_NONE)
>>   		return 0;
>>   	if (repr_id == NULL)
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 3b773b6..c1e5d4b 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -2702,6 +2702,7 @@ int rte_eth_allmulticast_get(uint16_t port_id);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if the function is not supported in PMD driver.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>   
>> @@ -2717,6 +2718,7 @@ int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if the function is not supported in PMD driver.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
>>   
>> @@ -2752,7 +2754,7 @@ const char *rte_eth_link_speed_to_str(uint32_t link_speed);
>>    * @param eth_link
>>    *   Link status returned by rte_eth_link_get function
>>    * @return
>> - *   Number of bytes written to str array.
>> + *   Number of bytes written to str array or -EINVAL if bad parameter.
>>    */
>>   __rte_experimental
>>   int rte_eth_link_to_str(char *str, size_t len,
>> @@ -2997,6 +2999,7 @@ int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>>    * @return
>>    *   - (0) if successful
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>   
>> @@ -3041,6 +3044,7 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>>   
>> @@ -3060,6 +3064,7 @@ int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>>    *   - (-ENOTSUP) if operation is not supported.
>>    *   - (-ENODEV) if *port_id* invalid.
>>    *   - (-EIO) if device is removed.
>> + *   - (-EINVAL) if bad parameter.
>>    *   - (>0) if *fw_size* is not enough to store firmware version, return
>>    *          the size of the non truncated string.
>>    */
>> @@ -3103,6 +3108,7 @@ int rte_eth_dev_fw_version_get(uint16_t port_id,
>>    *           only num entries will be filled into the ptypes array, but the full
>>    *           count of supported ptypes will be returned.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>   				     uint32_t *ptypes, int num);
>> @@ -3153,6 +3159,7 @@ int rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>    * @return
>>    *   - (0) if successful.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu);
>>   
>> @@ -3347,7 +3354,7 @@ rte_eth_tx_buffer_init(struct rte_eth_dev_tx_buffer *buffer, uint16_t size);
>>    * @param userdata
>>    *   Arbitrary parameter to be passed to the callback function
>>    * @return
>> - *   0 on success, or -1 on error with rte_errno set appropriately
>> + *   0 on success, or -EINVAL if bad parameter
>>    */
>>   int
>>   rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>> @@ -3774,6 +3781,7 @@ int rte_eth_fec_set(uint16_t port_id, uint32_t fec_capa);
>>    *   - (-ENOTSUP) if hardware doesn't support flow control.
>>    *   - (-ENODEV)  if *port_id* invalid.
>>    *   - (-EIO)  if device is removed.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_flow_ctrl_get(uint16_t port_id,
>>   			      struct rte_eth_fc_conf *fc_conf);
>> @@ -3845,7 +3853,8 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *mac_addr,
>>    *   - (0) if successful, or *mac_addr* didn't exist.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>>    *   - (-ENODEV) if *port* invalid.
>> - *   - (-EADDRINUSE) if attempting to remove the default MAC address
>> + *   - (-EADDRINUSE) if attempting to remove the default MAC address.
>> + *   - (-EINVAL) if MAC address is invalid.
>>    */
>>   int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>   				struct rte_ether_addr *mac_addr);
>> @@ -4044,6 +4053,7 @@ int rte_eth_dev_rss_hash_update(uint16_t port_id,
>>    *   - (-ENODEV) if port identifier is invalid.
>>    *   - (-EIO) if device is removed.
>>    *   - (-ENOTSUP) if hardware doesn't support RSS.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int
>>   rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> @@ -4112,6 +4122,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
>>    *   - (-ENODEV) if port identifier is invalid.
>>    *   - (-EIO) if device is removed.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_dcb_info(uint16_t port_id,
>>   			     struct rte_eth_dcb_info *dcb_info);
>> @@ -4628,6 +4639,7 @@ int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
>>    *
>>    * @return
>>    *   - 0: Success.
>> + *   - -EINVAL: Bad parameter.
>>    */
>>   int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
>>   
>> @@ -4694,6 +4706,7 @@ int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
>>    *   - 0: Success.
>>    *   - -ENODEV: The port ID is invalid.
>>    *   - -ENOTSUP: The function is not supported by the Ethernet driver.
>> + *   - -EINVAL: if bad parameter.
>>    */
>>   __rte_experimental
>>   int
>> @@ -4797,6 +4810,7 @@ rte_eth_dev_get_sec_ctx(uint16_t port_id);
>>    * @return
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   __rte_experimental
>>   int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>
> 
> .
>
  

Hi, Thomas,
	v6 has been sent to fix it, please check it out, thanks.

在 2021/4/15 20:15, Thomas Monjalon 写道:
> 15/04/2021 14:04, Kevin Traynor:
>> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>>> +	if (iter == NULL) {
>>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");
>>
>> "Failed to init iterator for NULL iterator\n"
> 
> The word "Failed" looks weird in these checks.
> What about "Cannot"?
> Example: "Cannot init NULL iterator"
> 
>>> +	if (devargs_str == NULL) {
>>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");
>>
>> "Failed to init iterator for NULL devargs\n"
> 
> "Cannot init iterator for NULL devargs"
> 
>>> +	if (owner == NULL) {
>>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
>>> +			port_id);
>>> +		return -EINVAL;
>>> +	}
>>
>> This fn uses both %u and %"PRIu16" for port_id
> 
> I don't see the benefit of PRIu16.
> 
>>> +	if (str == NULL) {
>>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");
>>
>> "Failed to convert link to NULL string\n"
> 
> "Cannot convert link in NULL string"
> 
>>> +	if (eth_link == NULL) {
>>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");
>>
>> "Failed to convert NULL link to string\n"
> 
> "Cannot convert NULL link"
> 
> 
> 
> .
>
  

On 16/04/2021 08:00, Min Hu (Connor) wrote:
> Thanks Kevin,
> 	all is fixed in v6, please review it, thanks.
> 	Some comments are below.
> 
> 在 2021/4/15 20:04, Kevin Traynor 写道:
>> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>>> This patch adds more sanity checks in control path APIs.
>>>
>>
>> Hi Connor,
>>
>> A few general comments,
>>
>> --
>> Some of the functions have unit tests, you could consider adding unit
>> tests for the new checks. Considering the checks are not subtle and
>> unlikely to be messed up in future, not adding unit tests is not a
>> blocker imho.
>>
>> --
>> It took me a while to get what you meant with "by NULL". It's usage
>> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
>> better way to phrase this generically, but I think it is more useful to
>> say what is NULL.
>>
>> e.g. "Failed to convert NULL to string\n" is very generic and would be
>> better as "Failed to convert NULL link to string\n" . ok, still a bit
>> generic but more of a clue.
>>
>> I won't comment on each log message individually but I've added a few
>> suggestions here and there.

Thanks, I think it looks a lot nicer to read in v6 my completely
subjective biased opinion :-)

>> --
>>
>> Did you check the usage of these functions in DPDK, and if the return
>> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
>> iterator functions. I'm not sure that having a return check is needed in
>> that case, but there could be other cases where you want to take some
>> different action now.
>>
> As iterator functions are all APIs, they may be used by APP directly.
> I think param check is necessary.

The point is that it would continue to call the functions even after it
caught this error, so would continue to print error messages. Yes, that
is much better than a seg fault and maybe in this case that is ok. I
will leave it to maintainers to decided.

I was just wondering if there was additional things similar to this in
DPDK where handling these new errors could now be improved too. I don't
think it has to be a prerequisite for this patch, as this patch is still
an improvement.

>> some other comments inlined,
>>
>>> 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")
  

在 2021/4/16 18:09, Kevin Traynor 写道:
> On 16/04/2021 08:00, Min Hu (Connor) wrote:
>> Thanks Kevin,
>> 	all is fixed in v6, please review it, thanks.
>> 	Some comments are below.
>>
>> 在 2021/4/15 20:04, Kevin Traynor 写道:
>>> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>>>> This patch adds more sanity checks in control path APIs.
>>>>
>>>
>>> Hi Connor,
>>>
>>> A few general comments,
>>>
>>> --
>>> Some of the functions have unit tests, you could consider adding unit
>>> tests for the new checks. Considering the checks are not subtle and
>>> unlikely to be messed up in future, not adding unit tests is not a
>>> blocker imho.
>>>
>>> --
>>> It took me a while to get what you meant with "by NULL". It's usage
>>> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
>>> better way to phrase this generically, but I think it is more useful to
>>> say what is NULL.
>>>
>>> e.g. "Failed to convert NULL to string\n" is very generic and would be
>>> better as "Failed to convert NULL link to string\n" . ok, still a bit
>>> generic but more of a clue.
>>>
>>> I won't comment on each log message individually but I've added a few
>>> suggestions here and there.
> 
> Thanks, I think it looks a lot nicer to read in v6 my completely
> subjective biased opinion :-)
> 
>>> --
>>>
>>> Did you check the usage of these functions in DPDK, and if the return
>>> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
>>> iterator functions. I'm not sure that having a return check is needed in
>>> that case, but there could be other cases where you want to take some
>>> different action now.
>>>
>> As iterator functions are all APIs, they may be used by APP directly.
>> I think param check is necessary.
> 
> The point is that it would continue to call the functions even after it
> caught this error, so would continue to print error messages. Yes, that
> is much better than a seg fault and maybe in this case that is ok. I
> will leave it to maintainers to decided.
> 
> I was just wondering if there was additional things similar to this in
> DPDK where handling these new errors could now be improved too. I don't
> think it has to be a prerequisite for this patch, as this patch is still
> an improvement.
> 
Thanks Kevin.
Well, for what your metioned, I will try to look for it.
If found, I will send another patch or initiate discussions.
Thanks.
>>> some other comments inlined,
>>>
>>>> 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")
> 
> .
>