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

  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(-)
  

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]
  

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.
  

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.
  

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.