在 2021/4/15 16:15, Andrew Rybchenko 写道:

On 4/15/21 3:52 AM, Min Hu (Connor) wrote:

quoted

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 <redacted>

quoted

Signed-off-by: Min Hu (Connor) <redacted>

[snip]

quoted

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

quoted

  	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.
.