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

[Min Hu (Connor)]

Hi, Ferruh,
        v6 has been sent to fix it, please check it out, thanks.

在 2021/4/15 23:38, Ferruh Yigit 写道:

On 4/15/2021 12:09 PM, 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

Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>

Hi Connor,

Please find a few comments below, with they are addressed:

Reviewed-by: Ferruh Yigit <ferruh.yi...@intel.com>

@@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
  int

  rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)

  {
-    int ret = 0;
-    struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
-
-    eth_dev_shared_data_prepare();
+    struct rte_eth_dev *ethdev;
-    rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);

Not sure '-1' is a good return value, what do you think sending '-ENODEV' as it is mostly done in the file?

Also emptly line can be removed as suggested by Andrew.

<...>

@@ -2138,8 +2181,14 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,

      int count;
      RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-
      dev = &rte_eth_devices[port_id];
+
+    if (conf == NULL) {

+        RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",

+            port_id);

When the line is long, can you break the message part to next line, to reduce the length at least a few columns, like:

RTE_ETHDEV_LOG(ERR,
     "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",
     port_id);

Same for all long messages.

<...>

@@ -5619,6 +5853,11 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,

      struct rte_eth_representor_info *info = NULL;
      size_t size;
+    if (ethdev == NULL) {

+        RTE_ETHDEV_LOG(ERR, "Failed to get device representor info from NULL\n");

+        return -EINVAL;
+    }
+

This is also an internal function, not sure if we should verify the 'ehtdev' here.

.