[PATCH v3] staging: wilc1000: Return correct error codes

[PATCH v3] staging: wilc1000: Return correct error codes

[Amitoj Kaur Chawla]

This change has been made with the goal that kernel functions should
return something more descriptive than -1 on failure.

The return value of alloc_etherdev on failure should return a -ENOMEM
and not a -1. This was found using Coccinelle. A simplified version of
the semantic patch used is:

//<smpl>
@@
expression *e;
identifier l1;
@@

e = alloc_etherdev(...);
if (e == NULL) {
...
return -1
+ -ENOMEM
;
}
//</smpl

Furthermore, return -ENODEV on failure of register_netdev() instead
of -1.

The two call sites store the return value in a variable which only
checks that the value is non-zero, hence no change is required at 
the call site.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
Changes in v3:
        -Further checked call sites to understand how the value is
         being analyzed
Changes in v2:
        -Corrected semantic patch

 drivers/staging/wilc1000/linux_wlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index a731b46..4f8b21d 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1333,7 +1333,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 	for (i = 0; i < NUM_CONCURRENT_IFC; i++) {
 		ndev = alloc_etherdev(sizeof(struct wilc_vif));
 		if (!ndev)
-			return -1;
+			return -ENOMEM;
 
 		vif = netdev_priv(ndev);
 		memset(vif, 0, sizeof(struct wilc_vif));
@@ -1373,7 +1373,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 		}
 
 		if (register_netdev(ndev))
-			return -1;
+			return -ENODEV;
 
 		vif->iftype = STATION_MODE;
 		vif->mac_opened = 0;
-- 
1.9.1

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Julia Lawall]

On Wed, 17 Feb 2016, Amitoj Kaur Chawla wrote:

> This change has been made with the goal that kernel functions should
> return something more descriptive than -1 on failure.
>
> The return value of alloc_etherdev on failure should return a -ENOMEM
> and not a -1. This was found using Coccinelle. A simplified version of
> the semantic patch used is:
>
> //<smpl>
> @@
> expression *e;
> identifier l1;
> @@
>
> e = alloc_etherdev(...);
> if (e == NULL) {
> ...
> return -1
> + -ENOMEM
> ;
> }
> //</smpl
>
> Furthermore, return -ENODEV on failure of register_netdev() instead
> of -1.

There is yet another failure that returns -1 in this function, on
wilc_create_wiphy.  That one should be fixed too.

Also, the call to the latter function is in a nested { }.  I guess this is
to be able to declare the variable wdev, but I don't see why this variable
can't be declared at the top of the function, like all the others.

> The two call sites store the return value in a variable which only
> checks that the value is non-zero, hence no change is required at
> the call site.

This is fine now.

julia

> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
> Changes in v3:
>         -Further checked call sites to understand how the value is
>          being analyzed
> Changes in v2:
>         -Corrected semantic patch
>
>  drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index a731b46..4f8b21d 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -1333,7 +1333,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  	for (i = 0; i < NUM_CONCURRENT_IFC; i++) {
>  		ndev = alloc_etherdev(sizeof(struct wilc_vif));
>  		if (!ndev)
> -			return -1;
> +			return -ENOMEM;
>
>  		vif = netdev_priv(ndev);
>  		memset(vif, 0, sizeof(struct wilc_vif));
> @@ -1373,7 +1373,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  		}
>
>  		if (register_netdev(ndev))
> -			return -1;
> +			return -ENODEV;
>
>  		vif->iftype = STATION_MODE;
>  		vif->mac_opened = 0;
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160217124204.GA30394%40amitoj-Inspiron-3542.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Arnd Bergmann]

On Wednesday 17 February 2016 18:12:05 Amitoj Kaur Chawla wrote:
> @@ -1373,7 +1373,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>                 }
>  
>                 if (register_netdev(ndev))
> -                       return -1;
> +                       return -ENODEV;
>  
>                 vif->iftype = STATION_MODE;
>                 vif->mac_opened = 0;
> -- 
> 

I would use whatever return code that register_netdev() returns here.

	Arnd

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Amitoj Kaur Chawla]

On Wed, Feb 17, 2016 at 6:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> I would use whatever return code that register_netdev() returns here.
>
>         Arnd

I can change it to that although I wrote the following script after
your comment to find the general pattern in the kernel:
@@
@@

if (register_netdev(...)) {
...
* return ...;
}

and most of the cases, there are quite some results in drivers/net/
return -ENODEV in the same scenario.

Would you still prefer to return the code that register_netdev() returns?

Amitoj

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Arnd Bergmann]

On Wednesday 17 February 2016 18:43:01 Amitoj Kaur Chawla wrote:
> On Wed, Feb 17, 2016 at 6:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I would use whatever return code that register_netdev() returns here.
> >
> >         Arnd
> 
> I can change it to that although I wrote the following script after
> your comment to find the general pattern in the kernel:
> @@
> @@
> 
> if (register_netdev(...)) {
> ...
> * return ...;
> }
> 
> and most of the cases, there are quite some results in drivers/net/
> return -ENODEV in the same scenario.
> 
> Would you still prefer to return the code that register_netdev() returns?
> 
> 

Yes, I think it's better: The reason for register_netdev failing is
almost certainly not that the device is not there, so -ENODEV is
a bit misleading.

	Arnd

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Amitoj Kaur Chawla]

On Wed, Feb 17, 2016 at 6:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> Yes, I think it's better: The reason for register_netdev failing is
> almost certainly not that the device is not there, so -ENODEV is
> a bit misleading.
>
>         Arnd

Okay. Will redo and send v4.

Thanks for the feedback,

Amitoj

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Julia Lawall]

On Wed, 17 Feb 2016, Arnd Bergmann wrote:

> On Wednesday 17 February 2016 18:43:01 Amitoj Kaur Chawla wrote:
> > On Wed, Feb 17, 2016 at 6:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > I would use whatever return code that register_netdev() returns here.
> > >
> > >         Arnd
> >
> > I can change it to that although I wrote the following script after
> > your comment to find the general pattern in the kernel:
> > @@
> > @@
> >
> > if (register_netdev(...)) {
> > ...
> > * return ...;
> > }

Amitoj, you can also write another script to see how many calls propagate
the error.  You will miss them with the above, because they don't have the
register_netdev call directly in the if test.  Maybe there are far more
that propagate than that follow the above pattern.

julia

> >
> > and most of the cases, there are quite some results in drivers/net/
> > return -ENODEV in the same scenario.
> >
> > Would you still prefer to return the code that register_netdev() returns?
> >
> >
>
> Yes, I think it's better: The reason for register_netdev failing is
> almost certainly not that the device is not there, so -ENODEV is
> a bit misleading.
>
> 	Arnd
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/3731725.Uhb0JIDMJg%40wuerfel.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v3] staging: wilc1000: Return correct error codes

[Amitoj Kaur Chawla]

On Wed, Feb 17, 2016 at 6:50 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> Amitoj, you can also write another script to see how many calls propagate
> the error.  You will miss them with the above, because they don't have the
> register_netdev call directly in the if test.  Maybe there are far more
> that propagate than that follow the above pattern.
>
> julia
>
Oh okay will do. I just wrote the above one quickly to find cases. I
can write a better one to check for all possible cases of
register_netdev and what is returned on failure.

Amitoj