[dpdk-dev] [PATCH] ethdev: fix error handling logic

[dpdk-dev] [PATCH] ethdev: fix error handling logic

[Alejandro Lucero]

This patch fixes how function exit is handled when errors inside
rte_eth_dev_create.

Fixes: e489007a411c ("ethdev: add generic create/destroy ethdev APIs")
Cc: stable@dpdk.org

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 lib/librte_ethdev/rte_ethdev.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index aa7730c..ef99f70 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -3467,10 +3467,8 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		ethdev = rte_eth_dev_allocate(name);
-		if (!ethdev) {
-			retval = -ENODEV;
-			goto probe_failed;
-		}
+		if (!ethdev)
+			return -ENODEV;
 
 		if (priv_data_size) {
 			ethdev->data->dev_private = rte_zmalloc_socket(
@@ -3480,7 +3478,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 			if (!ethdev->data->dev_private) {
 				RTE_LOG(ERR, EAL, "failed to allocate private data");
 				retval = -ENOMEM;
-				goto probe_failed;
+				goto data_alloc_failed;
 			}
 		}
 	} else {
@@ -3488,8 +3486,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 		if (!ethdev) {
 			RTE_LOG(ERR, EAL, "secondary process attach failed, "
 				"ethdev doesn't exist");
-			retval = -ENODEV;
-			goto probe_failed;
+			return  -ENODEV;
 		}
 	}
 
@@ -3518,6 +3515,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		rte_free(ethdev->data->dev_private);
 
+data_alloc_failed:
 	rte_eth_dev_release_port(ethdev);
 
 	return retval;
-- 
1.9.1

Re: [dpdk-dev] [PATCH] ethdev: fix error handling logic

[Andrew Rybchenko]

On 9/24/18 4:43 PM, Alejandro Lucero wrote:
> This patch fixes how function exit is handled when errors inside
> rte_eth_dev_create.
>
> Fixes: e489007a411c ("ethdev: add generic create/destroy ethdev APIs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Minor nit/observation below, but anyway

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

> ---
>   lib/librte_ethdev/rte_ethdev.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index aa7730c..ef99f70 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3467,10 +3467,8 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   
>   	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>   		ethdev = rte_eth_dev_allocate(name);
> -		if (!ethdev) {
> -			retval = -ENODEV;
> -			goto probe_failed;
> -		}
> +		if (!ethdev)
> +			return -ENODEV;

As far as I can see rte_eth_dev_allocate() returns NULL if a device
with such name already exists or no free ports left. I'd say that
EEXIST and ENOSPC better describe what went wrong, but the patch
simply does not change it and there is no easy way to fix it
(except may be rte_errno usage).

>   
>   		if (priv_data_size) {
>   			ethdev->data->dev_private = rte_zmalloc_socket(
> @@ -3480,7 +3478,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   			if (!ethdev->data->dev_private) {
>   				RTE_LOG(ERR, EAL, "failed to allocate private data");
>   				retval = -ENOMEM;
> -				goto probe_failed;
> +				goto data_alloc_failed;
>   			}
>   		}
>   	} else {
> @@ -3488,8 +3486,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   		if (!ethdev) {
>   			RTE_LOG(ERR, EAL, "secondary process attach failed, "
>   				"ethdev doesn't exist");
> -			retval = -ENODEV;
> -			goto probe_failed;
> +			return  -ENODEV;

Here ENODEV is 100% correct since secondary simply failed to find
ethdev with matching name.

>   		}
>   	}
>   
> @@ -3518,6 +3515,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>   		rte_free(ethdev->data->dev_private);
>   
> +data_alloc_failed:
>   	rte_eth_dev_release_port(ethdev);
>   
>   	return retval;

Re: [dpdk-dev] [dpdk-stable] [PATCH] ethdev: fix error handling logic

[Ferruh Yigit]

On 9/24/2018 3:10 PM, Andrew Rybchenko wrote:
> On 9/24/18 4:43 PM, Alejandro Lucero wrote:
>> This patch fixes how function exit is handled when errors inside
>> rte_eth_dev_create.
>>
>> Fixes: e489007a411c ("ethdev: add generic create/destroy ethdev APIs")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> 
> Minor nit/observation below, but anyway
> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Applied to dpdk-next-net/master, thanks.