[dpdk-dev] [PATCH v2 02/54] ethdev: change rte_eth_dev_info_get() return value to int

[Andrew Rybchenko]

On 9/4/19 7:40 PM, Ferruh Yigit wrote:
> On 9/3/2019 2:56 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko at oktetlabs.com>
>>
>> Change rte_eth_dev_info_get() return value from void to int and return
>> negative errno values in case of error conditions.
>> Modify rte_eth_dev_info_get() usage across the ethdev according
>> to new return type.
>>
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko at oktetlabs.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
> <...>
>
>> @@ -1144,7 +1143,9 @@ struct rte_eth_dev *
> The diff output seems not able to detect the function/block which makes it
> harder to review, same patch I am getting a diff output like below:
>   @@ -1144,7 +1143,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q,
> uint16_t nb_tx_q,
>
> Can you please either check a newer version of git, as far as I can see you are
> using '1.8.3.1', or can you please try by removing/updating '.gitattributes' file?

Newer Git version solves the problem.

>>   	 */
>>   	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>   
>> -	rte_eth_dev_info_get(port_id, &dev_info);
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> 'dev->data->dev_conf' should be restored from 'orig_conf' on failure, before return.

Thanks, will fix in v3.

> <...>
>
>> -void
>> +int
>>   rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   {
>>   	struct rte_eth_dev *dev;
>> @@ -2558,7 +2566,7 @@ struct rte_eth_dev *
>>   	 */
>>   	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>>   
>> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	dev_info->rx_desc_lim = lim;
>> @@ -2567,13 +2575,15 @@ struct rte_eth_dev *
>>   	dev_info->min_mtu = RTE_ETHER_MIN_MTU;
>>   	dev_info->max_mtu = UINT16_MAX;
>>   
>> -	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>>   	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
>>   	dev_info->driver_name = dev->device->driver->name;
>>   	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>>   	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>>   
>>   	dev_info->dev_flags = &dev->data->dev_flags;
>> +
>> +	return 0;
> Should API use "return eth_err(port_id, ret);" syntax, as other APIs do,
> I am not very clear about the 'eth_err()', Thomas can provide better
> description/suggestion on this.

In this particular case eth_err() is irrelevant. It makes sense when
callback is updated to return int and I'll use it there.
In any case better description would be useful.

> <...>
>
>> @@ -3100,9 +3118,13 @@ struct rte_eth_dev *
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>   	unsigned i;
>> +	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> -	rte_eth_dev_info_get(port_id, &dev_info);
>> +
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> This is 'get_mac_addr_index()' static function, and it should either send
> negative value for error, or 'index' value in case of success. So should we be
> sure here only negative value sent? Incase 'rte_eth_dev_info_get()' returns
> positive value in the future.

In fact, I think there is no necessity to check port_id here since it is 
a static
function and port_id is now checked in rte_eth_dev_info_get() anyway.
Also, I think it is better to return -1 here since it is bad to mix 
-errno and -1
return codes. -errno would be useful if caller really takes it into 
account and
have different processing for -ENOENT which makes sense in this case.

>>   
>>   	for (i = 0; i < dev_info.max_mac_addrs; i++)
>>   		if (memcmp(addr, &dev->data->mac_addrs[i],
>> @@ -3233,8 +3255,14 @@ struct rte_eth_dev *
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>   	unsigned i;
>> +	int ret;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> Same comment as above, for this 'get_hash_mac_addr_index()' static function.

Same as above.

Many thanks for review.