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: sta...@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.rybche...@oktetlabs.ru>
>
> Signed-off-by: Min Hu (Connor) <humi...@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.
