[dpdk-dev] [PATCH 0/2] minor cleanup in ethdev hotplug

[dpdk-dev] [PATCH 0/2] minor cleanup in ethdev hotplug

[David Marchand]

It was first a preparation step for future patchsets, but I am not sure what
will become of them, so sending this anyway since it does not hurt to clean
this now.


-- 
David Marchand

David Marchand (2):
  ethdev: remove useless checks
  ethdev: move code to common place in hotplug

 lib/librte_ether/rte_ethdev.c | 84 +++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 44 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH 1/2] ethdev: remove useless null checks

[David Marchand]

We are in static functions and those passed arguments can't be NULL.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index af990e2..951fb1c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -220,9 +220,6 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
 {
 	int ret;
 
-	if ((name == NULL) || (pci_dev == NULL))
-		return -EINVAL;
-
 	ret = snprintf(name, size, "%d:%d.%d",
 			pci_dev->addr.bus, pci_dev->addr.devid,
 			pci_dev->addr.function);
@@ -505,9 +502,6 @@ rte_eth_dev_is_detachable(uint8_t port_id)
 static int
 rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
 {
-	if ((addr == NULL) || (port_id == NULL))
-		goto err;
-
 	/* re-construct pci_device_list */
 	if (rte_eal_pci_scan())
 		goto err;
@@ -531,9 +525,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	struct rte_pci_addr freed_addr;
 	struct rte_pci_addr vp;
 
-	if (addr == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
@@ -566,9 +557,6 @@ rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
 	char *name = NULL, *args = NULL;
 	int ret = -1;
 
-	if ((vdevargs == NULL) || (port_id == NULL))
-		goto end;
-
 	/* parse vdevargs, then retrieve device name and args */
 	if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
 		goto end;
@@ -600,9 +588,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
-	if (vdevname == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
-- 
1.9.1

Re: [dpdk-dev] [dpdk-dev,1/2] ethdev: remove useless null checks

[Jan Viktorin]

On Thu, 21 Jan 2016 12:57:10 +0100
David Marchand <david.marchand@6wind.com> wrote:

> We are in static functions and those passed arguments can't be NULL.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> ---
> lib/librte_ether/rte_ethdev.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index af990e2..951fb1c 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -220,9 +220,6 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
>  {
>  	int ret;
>  
> -	if ((name == NULL) || (pci_dev == NULL))
> -		return -EINVAL;

Do you use a kind of assert in DPDK? The patch looks OK, however, I
would prefer something like

	assert_not_null(name);
	assert_not_null(pci_dev);

Usually, if some outer code is broken by mistake, the assert catches
such an issue. At the same time, it documents the code by telling
"this must never be NULL here". I agree, that returning -EINVAL for
this kind of check is incorrect.

Same for other changes...

> [snip]

Re: [dpdk-dev] [dpdk-dev,1/2] ethdev: remove useless null checks

[Thomas Monjalon]

2016-01-21 20:02, Jan Viktorin:
> On Thu, 21 Jan 2016 12:57:10 +0100
> David Marchand <david.marchand@6wind.com> wrote:
> > -	if ((name == NULL) || (pci_dev == NULL))
> > -		return -EINVAL;
> 
> Do you use a kind of assert in DPDK? The patch looks OK, however, I
> would prefer something like
> 
> 	assert_not_null(name);
> 	assert_not_null(pci_dev);
> 
> Usually, if some outer code is broken by mistake, the assert catches
> such an issue. At the same time, it documents the code by telling
> "this must never be NULL here". I agree, that returning -EINVAL for
> this kind of check is incorrect.
> 
> Same for other changes...

For this patch, I agree to remove useless checks.
For the other checks, EINVAL seems to be the right error code.
And yes you are right, we could replace most of EINVAL returns by a kind
of assert. RTE_VERIFY would be appropriate.

[dpdk-dev] [PATCH 2/2] ethdev: move code to common place in hotplug

[David Marchand]

Move these error logs and checks on detach capabilities in a common place.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 69 +++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 951fb1c..9083783 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
 
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
 	return -1;
 }
 
@@ -525,10 +524,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	struct rte_pci_addr freed_addr;
 	struct rte_pci_addr vp;
 
-	/* check whether the driver supports detach feature, or not */
-	if (rte_eth_dev_is_detachable(port_id))
-		goto err;
-
 	/* get pci address by port id */
 	if (rte_eth_dev_get_addr_by_port(port_id, &freed_addr))
 		goto err;
@@ -546,7 +541,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	*addr = freed_addr;
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
 	return -1;
 }
 
@@ -577,8 +571,6 @@ end:
 	free(name);
 	free(args);
 
-	if (ret < 0)
-		RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
 	return ret;
 }
 
@@ -588,10 +580,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
-	/* check whether the driver supports detach feature, or not */
-	if (rte_eth_dev_is_detachable(port_id))
-		goto err;
-
 	/* get device name by port id */
 	if (rte_eth_dev_get_name_by_port(port_id, name))
 		goto err;
@@ -603,7 +591,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 	strncpy(vdevname, name, sizeof(name));
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
 	return -1;
 }
 
@@ -612,14 +599,25 @@ int
 rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
 {
 	struct rte_pci_addr addr;
+	int ret = -1;
 
 	if ((devargs == NULL) || (port_id == NULL))
-		return -EINVAL;
+		goto err;
 
-	if (eal_parse_pci_DomBDF(devargs, &addr) == 0)
-		return rte_eth_dev_attach_pdev(&addr, port_id);
-	else
-		return rte_eth_dev_attach_vdev(devargs, port_id);
+	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
+		ret = rte_eth_dev_attach_pdev(&addr, port_id);
+		if (ret < 0)
+			goto err;
+	} else {
+		ret = rte_eth_dev_attach_vdev(devargs, port_id);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
+	return ret;
 }
 
 /* detach the device, then store the name of the device */
@@ -627,26 +625,39 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = -1;
 
 	if (name == NULL)
-		return -EINVAL;
+		goto err;
+
+	/* check whether the driver supports detach feature, or not */
+	if (rte_eth_dev_is_detachable(port_id))
+		goto err;
 
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
-			return ret;
+			goto err;
 
 		ret = rte_eth_dev_detach_pdev(port_id, &addr);
-		if (ret == 0)
-			snprintf(name, RTE_ETH_NAME_MAX_LEN,
-				"%04x:%02x:%02x.%d",
-				addr.domain, addr.bus,
-				addr.devid, addr.function);
+		if (ret < 0)
+			goto err;
 
-		return ret;
-	} else
-		return rte_eth_dev_detach_vdev(port_id, name);
+		snprintf(name, RTE_ETH_NAME_MAX_LEN,
+			"%04x:%02x:%02x.%d",
+			addr.domain, addr.bus,
+			addr.devid, addr.function);
+	} else {
+		ret = rte_eth_dev_detach_vdev(port_id, name);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
+	return ret;
 }
 
 static int
-- 
1.9.1

Re: [dpdk-dev] [dpdk-dev, 2/2] ethdev: move code to common place in hotplug

[Jan Viktorin]

On Thu, 21 Jan 2016 12:57:11 +0100
David Marchand <david.marchand@6wind.com> wrote:

> Move these error logs and checks on detach capabilities in a common place.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> ---
> lib/librte_ether/rte_ethdev.c | 69 +++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 951fb1c..9083783 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>  
>  	return 0;
> [snip]
>  
> @@ -612,14 +599,25 @@ int
>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
>  {
>  	struct rte_pci_addr addr;
> +	int ret = -1;
>  
>  	if ((devargs == NULL) || (port_id == NULL))
> -		return -EINVAL;
> +		goto err;

This change modifies the return value from -EINVAL to -1. I don't know
whether is this an issue but it looks suspicious.

>  
> -	if (eal_parse_pci_DomBDF(devargs, &addr) == 0)
> -		return rte_eth_dev_attach_pdev(&addr, port_id);
> -	else
> -		return rte_eth_dev_attach_vdev(devargs, port_id);
> +	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
> +		ret = rte_eth_dev_attach_pdev(&addr, port_id);
> +		if (ret < 0)
> +			goto err;
> +	} else {
> +		ret = rte_eth_dev_attach_vdev(devargs, port_id);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> +	return ret;
>  }
>  
>  /* detach the device, then store the name of the device */
> @@ -627,26 +625,39 @@ int
>  rte_eth_dev_detach(uint8_t port_id, char *name)
>  {
>  	struct rte_pci_addr addr;
> -	int ret;
> +	int ret = -1;
>  
>  	if (name == NULL)
> -		return -EINVAL;
> +		goto err;

Same here...

> +
> +	/* check whether the driver supports detach feature, or not */
> +	if (rte_eth_dev_is_detachable(port_id))
> +		goto err;
>  
> [snip]




-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

Re: [dpdk-dev] [dpdk-dev, 2/2] ethdev: move code to common place in hotplug

[David Marchand]

On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
> On Thu, 21 Jan 2016 12:57:11 +0100
> David Marchand <david.marchand@6wind.com> wrote:
>
[snip]
>> @@ -612,14 +599,25 @@ int
>>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
>>  {
>>       struct rte_pci_addr addr;
>> +     int ret = -1;
>>
>>       if ((devargs == NULL) || (port_id == NULL))
>> -             return -EINVAL;
>> +             goto err;
>
> This change modifies the return value from -EINVAL to -1. I don't know
> whether is this an issue but it looks suspicious.

Should not be an issue, as the api does not give details on expected
negative return values.
Just noticed, this also introduces a new log message that was not
displayed before.

To be safe, I suppose I should restore this.

Thomas, opinion ?


Thanks.
-- 
David Marchand

Re: [dpdk-dev] [dpdk-dev, 2/2] ethdev: move code to common place in hotplug

[Thomas Monjalon]

2016-01-21 19:06, David Marchand:
> On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
> > On Thu, 21 Jan 2016 12:57:11 +0100
> > David Marchand <david.marchand@6wind.com> wrote:
> >
> [snip]
> >> @@ -612,14 +599,25 @@ int
> >>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
> >>  {
> >>       struct rte_pci_addr addr;
> >> +     int ret = -1;
> >>
> >>       if ((devargs == NULL) || (port_id == NULL))
> >> -             return -EINVAL;
> >> +             goto err;
> >
> > This change modifies the return value from -EINVAL to -1. I don't know
> > whether is this an issue but it looks suspicious.
> 
> Should not be an issue, as the api does not give details on expected
> negative return values.
> Just noticed, this also introduces a new log message that was not
> displayed before.
> 
> To be safe, I suppose I should restore this.
> 
> Thomas, opinion ?

I'm OK with the log message added for this error case.
I would just keep the -EINVAL return value.

Other nit: you are allowed to fix the (moved) log message.

Re: [dpdk-dev] [dpdk-dev, 2/2] ethdev: move code to common place in hotplug

[David Marchand]

Hello,

On Thu, Jan 21, 2016 at 7:42 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-21 19:06, David Marchand:
>> On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
>> > This change modifies the return value from -EINVAL to -1. I don't know
>> > whether is this an issue but it looks suspicious.
>>
>> Should not be an issue, as the api does not give details on expected
>> negative return values.
>> Just noticed, this also introduces a new log message that was not
>> displayed before.
>>
>> To be safe, I suppose I should restore this.
>>
>> Thomas, opinion ?
>
> I'm OK with the log message added for this error case.
> I would just keep the -EINVAL return value.

Ok will do.


-- 
David Marchand

[dpdk-dev] [PATCH v2 0/2] minor cleanup in ethdev hotplug

[David Marchand]

It was first a preparation step for future patchsets, but I am not sure what
will become of them, so sending this anyway since it does not hurt to clean
this now.

Changes since v1:
- rebased on HEAD (previous patchset was based on another patch I sent
  separately)
- restored EINVAL error code for rte_eth_dev_(at|de)tach (thanks Jan)


-- 
David Marchand

David Marchand (2):
  ethdev: remove useless null checks
  ethdev: move code to common place in hotplug

 lib/librte_ether/rte_ethdev.c | 92 +++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v2 1/2] ethdev: remove useless null checks

[David Marchand]

We are in static functions and those passed arguments can't be NULL.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ed971b4..cab74e0 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -220,9 +220,6 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
 {
 	int ret;
 
-	if ((name == NULL) || (pci_dev == NULL))
-		return -EINVAL;
-
 	ret = snprintf(name, size, "%d:%d.%d",
 			pci_dev->addr.bus, pci_dev->addr.devid,
 			pci_dev->addr.function);
@@ -505,9 +502,6 @@ rte_eth_dev_is_detachable(uint8_t port_id)
 static int
 rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
 {
-	if ((addr == NULL) || (port_id == NULL))
-		goto err;
-
 	/* re-construct pci_device_list */
 	if (rte_eal_pci_scan())
 		goto err;
@@ -531,9 +525,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	struct rte_pci_addr freed_addr;
 	struct rte_pci_addr vp;
 
-	if (addr == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
@@ -566,9 +557,6 @@ rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
 	char *name = NULL, *args = NULL;
 	int ret = -1;
 
-	if ((vdevargs == NULL) || (port_id == NULL))
-		goto end;
-
 	/* parse vdevargs, then retrieve device name and args */
 	if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
 		goto end;
@@ -602,9 +590,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
-	if (vdevname == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: remove useless null checks

[Jan Viktorin]

What about the RTE_VERIFY? I think, it's more appropriate here.
Otherwise, feel free to add:

Reviewed-by: Jan Viktorin <viktorin@rehivetech.com>

On Fri, 22 Jan 2016 15:06:57 +0100
David Marchand <david.marchand@6wind.com> wrote:

> We are in static functions and those passed arguments can't be NULL.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ed971b4..cab74e0 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -220,9 +220,6 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
>  {
>  	int ret;
>  
> -	if ((name == NULL) || (pci_dev == NULL))
> -		return -EINVAL;
> -
>  	ret = snprintf(name, size, "%d:%d.%d",
>  			pci_dev->addr.bus, pci_dev->addr.devid,
>  			pci_dev->addr.function);
> @@ -505,9 +502,6 @@ rte_eth_dev_is_detachable(uint8_t port_id)
>  static int
>  rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>  {
> -	if ((addr == NULL) || (port_id == NULL))
> -		goto err;
> -
>  	/* re-construct pci_device_list */
>  	if (rte_eal_pci_scan())
>  		goto err;
> @@ -531,9 +525,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
>  	struct rte_pci_addr freed_addr;
>  	struct rte_pci_addr vp;
>  
> -	if (addr == NULL)
> -		goto err;
> -
>  	/* check whether the driver supports detach feature, or not */
>  	if (rte_eth_dev_is_detachable(port_id))
>  		goto err;
> @@ -566,9 +557,6 @@ rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
>  	char *name = NULL, *args = NULL;
>  	int ret = -1;
>  
> -	if ((vdevargs == NULL) || (port_id == NULL))
> -		goto end;
> -
>  	/* parse vdevargs, then retrieve device name and args */
>  	if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
>  		goto end;
> @@ -602,9 +590,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
>  {
>  	char name[RTE_ETH_NAME_MAX_LEN];
>  
> -	if (vdevname == NULL)
> -		goto err;
> -
>  	/* check whether the driver supports detach feature, or not */
>  	if (rte_eth_dev_is_detachable(port_id))
>  		goto err;



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: remove useless null checks

[David Marchand]

On Tue, Jan 26, 2016 at 4:50 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
> What about the RTE_VERIFY? I think, it's more appropriate here.

Well, here, I am removing useless checks in static functions.

But for the rest of ethdev api, I agree we could add some RTE_VERIFY.

> Otherwise, feel free to add:
>
> Reviewed-by: Jan Viktorin <viktorin@rehivetech.com>

Thanks.


-- 
David Marchand

[dpdk-dev] [PATCH v2 2/2] ethdev: move code to common place in hotplug

[David Marchand]

Move these error logs and checks on detach capabilities in a common place.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
Changes since v1:
- restore EINVAL error code for rte_eth_dev_(at|de)tach

 lib/librte_ether/rte_ethdev.c | 77 ++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 31 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index cab74e0..a45563d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
 
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
 	return -1;
 }
 
@@ -525,10 +524,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	struct rte_pci_addr freed_addr;
 	struct rte_pci_addr vp;
 
-	/* check whether the driver supports detach feature, or not */
-	if (rte_eth_dev_is_detachable(port_id))
-		goto err;
-
 	/* get pci address by port id */
 	if (rte_eth_dev_get_addr_by_port(port_id, &freed_addr))
 		goto err;
@@ -546,7 +541,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	*addr = freed_addr;
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
 	return -1;
 }
 
@@ -579,8 +573,6 @@ end:
 	if (args)
 		free(args);
 
-	if (ret < 0)
-		RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
 	return ret;
 }
 
@@ -590,10 +582,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
-	/* check whether the driver supports detach feature, or not */
-	if (rte_eth_dev_is_detachable(port_id))
-		goto err;
-
 	/* get device name by port id */
 	if (rte_eth_dev_get_name_by_port(port_id, name))
 		goto err;
@@ -605,7 +593,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 	strncpy(vdevname, name, sizeof(name));
 	return 0;
 err:
-	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
 	return -1;
 }
 
@@ -614,14 +601,27 @@ int
 rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
 {
 	struct rte_pci_addr addr;
+	int ret = -1;
 
-	if ((devargs == NULL) || (port_id == NULL))
-		return -EINVAL;
+	if ((devargs == NULL) || (port_id == NULL)) {
+		ret = -EINVAL;
+		goto err;
+	}
 
-	if (eal_parse_pci_DomBDF(devargs, &addr) == 0)
-		return rte_eth_dev_attach_pdev(&addr, port_id);
-	else
-		return rte_eth_dev_attach_vdev(devargs, port_id);
+	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
+		ret = rte_eth_dev_attach_pdev(&addr, port_id);
+		if (ret < 0)
+			goto err;
+	} else {
+		ret = rte_eth_dev_attach_vdev(devargs, port_id);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
+	return ret;
 }
 
 /* detach the device, then store the name of the device */
@@ -629,26 +629,41 @@ int
 rte_eth_dev_detach(uint8_t port_id, char *name)
 {
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = -1;
 
-	if (name == NULL)
-		return -EINVAL;
+	if (name == NULL) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* check whether the driver supports detach feature, or not */
+	if (rte_eth_dev_is_detachable(port_id))
+		goto err;
 
 	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
 		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
 		if (ret < 0)
-			return ret;
+			goto err;
 
 		ret = rte_eth_dev_detach_pdev(port_id, &addr);
-		if (ret == 0)
-			snprintf(name, RTE_ETH_NAME_MAX_LEN,
-				"%04x:%02x:%02x.%d",
-				addr.domain, addr.bus,
-				addr.devid, addr.function);
+		if (ret < 0)
+			goto err;
 
-		return ret;
-	} else
-		return rte_eth_dev_detach_vdev(port_id, name);
+		snprintf(name, RTE_ETH_NAME_MAX_LEN,
+			"%04x:%02x:%02x.%d",
+			addr.domain, addr.bus,
+			addr.devid, addr.function);
+	} else {
+		ret = rte_eth_dev_detach_vdev(port_id, name);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
+	return ret;
 }
 
 static int
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: move code to common place in hotplug

[Jan Viktorin]

On Fri, 22 Jan 2016 15:06:58 +0100
David Marchand <david.marchand@6wind.com> wrote:

> Move these error logs and checks on detach capabilities in a common place.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
Reviewed-by: Jan Viktorin <viktorin@rehivetech.com>

Re: [dpdk-dev] [PATCH v2 0/2] minor cleanup in ethdev hotplug

[Thomas Monjalon]

2016-01-22 15:06, David Marchand:
> It was first a preparation step for future patchsets, but I am not sure what
> will become of them, so sending this anyway since it does not hurt to clean
> this now.
> 
> Changes since v1:
> - rebased on HEAD (previous patchset was based on another patch I sent
>   separately)
> - restored EINVAL error code for rte_eth_dev_(at|de)tach (thanks Jan)

Applied, thanks