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

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