[dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs
Min Hu (Connor)
humin29 at huawei.com
Thu Apr 15 13:09:45 CEST 2021
在 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.
> .
>
More information about the dev
mailing list