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

["Min Hu (Connor)" <humin29@huawei.com>]

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: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
> Hi Connor,
> 
> Please find a few comments below, with they are addressed:
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@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.
> .