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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/10/2021 10:18 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
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Thanks Connor for the patch, please find a few minor comments below.

<...>

> @@ -3822,6 +3903,9 @@ rte_eth_dev_rss_reta_query(uint16_t port_id,
>   	struct rte_eth_dev *dev;
>   	int ret;
>   
> +	if (reta_conf == NULL)
> +		return -EINVAL;
> +

The 'eth_check_reta_mask()' & 'eth_check_reta_entry()' static fucntions are also 
checking 'reta_conf' against NULL, they may be removed after these checks.

<...>

> @@ -4447,6 +4552,9 @@ rte_eth_dev_callback_process(struct rte_eth_dev *dev,
>   	struct rte_eth_dev_callback dev_cb;
>   	int rc = 0;
>   
> +	if (dev == NULL)
> +		return -EINVAL;
> +

This is internal API, not sure if needed to verify 'dev' here, it won't hurt of 
course, but just to prevent unnecessary checks. No strong option.

Same comment for all internal functions that changed.

<...>

> @@ -5370,6 +5502,9 @@ rte_eth_dev_get_dcb_info(uint16_t port_id,
>   {
>   	struct rte_eth_dev *dev;
>   
> +	if (dcb_info == NULL)
> +		return -EINVAL;
> +

Function documentation in the header file, doesn't list '-EINVAL' as a return 
value, whenever a new return value is returned, it needs to be documented.