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

[Min Hu (Connor)]

在 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 at 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 at oktetlabs.ru>
> 
>>
>> Signed-off-by: Min Hu (Connor) <humin29 at 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.
> .
>