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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/3/2019 2:56 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@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@oktetlabs.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@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?

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

<...>

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

<...>

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

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