[PATCH] staging: rtl8723au : remove goto & return error directly

[PATCH] staging: rtl8723au : remove goto & return error directly

[Gujulan Elango, Hari Prasath (H.)]

Remove the goto and return error directly thereby removing a variable

Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
---
 drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
index b139ed4..a979048 100644
--- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
@@ -1209,7 +1209,6 @@ static int cfg80211_rtw_change_iface(struct wiphy *wiphy,
 	struct rtw_adapter *padapter = wiphy_to_adapter(wiphy);
 	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
 	struct wireless_dev *rtw_wdev = wiphy_to_wdev(wiphy);
-	int ret = 0;
 
 	DBG_8723A("%s(%s): call netdev_open23a\n", __func__, ndev->name);
 
@@ -1238,14 +1237,12 @@ static int cfg80211_rtw_change_iface(struct wiphy *wiphy,
 
 	if (cfg80211_infrastructure_mode(padapter, type) != _SUCCESS) {
 		rtw_wdev->iftype = old_type;
-		ret = -EPERM;
-		goto exit;
+		return -EPERM;
 	}
 
 	rtw_setopmode_cmd23a(padapter, type);
 
-exit:
-	return ret;
+	return 0;
 }
 
 void rtw_cfg80211_indicate_scan_done(struct rtw_wdev_priv *pwdev_priv,
-- 
1.9.1

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Jes Sorensen]

"Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> Remove the goto and return error directly thereby removing a variable
>
> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> ---
>  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

NACK!

The goto is cleaner and more consistent, rather than hiding a return in
the middle of the code.

Jes

>
> diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> index b139ed4..a979048 100644
> --- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> @@ -1209,7 +1209,6 @@ static int cfg80211_rtw_change_iface(struct wiphy *wiphy,
>  	struct rtw_adapter *padapter = wiphy_to_adapter(wiphy);
>  	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
>  	struct wireless_dev *rtw_wdev = wiphy_to_wdev(wiphy);
> -	int ret = 0;
>  
>  	DBG_8723A("%s(%s): call netdev_open23a\n", __func__, ndev->name);
>  
> @@ -1238,14 +1237,12 @@ static int cfg80211_rtw_change_iface(struct wiphy *wiphy,
>  
>  	if (cfg80211_infrastructure_mode(padapter, type) != _SUCCESS) {
>  		rtw_wdev->iftype = old_type;
> -		ret = -EPERM;
> -		goto exit;
> +		return -EPERM;
>  	}
>  
>  	rtw_setopmode_cmd23a(padapter, type);
>  
> -exit:
> -	return ret;
> +	return 0;
>  }
>  
>  void rtw_cfg80211_indicate_scan_done(struct rtw_wdev_priv *pwdev_priv,

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Dan Carpenter]

On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
> "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> > Remove the goto and return error directly thereby removing a variable
> >
> > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> > ---
> >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> NACK!

SHOUTING!

> 
> The goto is cleaner and more consistent, rather than hiding a return in
> the middle of the code.

More consistent with what?  There are already returns earlier in the
function.  Do-nothing gotos are pointless and annoying.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Gujulan Elango, Hari Prasath (H.)]

On Thu, May 07, 2015 at 03:19:34PM +0300, Dan Carpenter wrote:
> On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
> > "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> > > Remove the goto and return error directly thereby removing a variable
> > >
> > > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> > > ---
> > >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > NACK!
> 
> SHOUTING!
> 
> > 
> > The goto is cleaner and more consistent, rather than hiding a return in
> > the middle of the code.
> 
> More consistent with what?  There are already returns earlier in the
> function.  Do-nothing gotos are pointless and annoying.
> 
> regards,
> dan carpenter
> 

I sent this patch  because goto & return was mixed up in this function.
There was a return as well as goto used. And i guess goto is not 
encouraged anywhere. In this case especially,I couldn't see any 
significant cleanup being done in the goto label.

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Jes Sorensen]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
>> "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
>> > Remove the goto and return error directly thereby removing a variable
>> >
>> > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>> > ---
>> >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> 
>> NACK!
>
> SHOUTING!

It's normal to write NACK in upper case
>
>> 
>> The goto is cleaner and more consistent, rather than hiding a return in
>> the middle of the code.
>
> More consistent with what?  There are already returns earlier in the
> function.  Do-nothing gotos are pointless and annoying.

That is *your* preference!

Having returns scattered randomly throughout the code is annoying and
hard to catch when reading the code!

This case is *not* a bug, so it comes down to personal preference of the
maintainer. For code you maintain, feel free to do as you like, I will
*not* never bother you for this kind of preference issues, I will tell
you if I spot an actual bug.

This is one of the problems with checkpatch style tools, they have
become harassment tools, for some people to impose their personal
preferences onto others.

It would be so much more valuable if they just focused on real bugs!

Jes

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Jes Sorensen]

"Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> On Thu, May 07, 2015 at 03:19:34PM +0300, Dan Carpenter wrote:
>> On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
>> > "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
>> > > Remove the goto and return error directly thereby removing a variable
>> > >
>> > > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>> > > ---
>> > >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
>> > >  1 file changed, 2 insertions(+), 5 deletions(-)
>> > 
>> > NACK!
>> 
>> SHOUTING!
>> 
>> > 
>> > The goto is cleaner and more consistent, rather than hiding a return in
>> > the middle of the code.
>> 
>> More consistent with what?  There are already returns earlier in the
>> function.  Do-nothing gotos are pointless and annoying.
>> 
>> regards,
>> dan carpenter
>> 
>
> I sent this patch  because goto & return was mixed up in this function.
> There was a return as well as goto used. And i guess goto is not 
> encouraged anywhere. In this case especially,I couldn't see any 
> significant cleanup being done in the goto label.

I will accept a patch to remove the returns in the middle of the
function, if you wish to submit that.

A return at the very beginning of a function is OK, but in the middle of
a larger function they make it harder to catch when code exists.

Jes

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Joe Perches]

On Thu, 2015-05-07 at 10:08 -0400, Jes Sorensen wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
> >> NACK!
> > SHOUTING!
> It's normal to write NACK in upper case

Not with an exclamation point.

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Jes Sorensen]

Joe Perches <joe@perches.com> writes:
> On Thu, 2015-05-07 at 10:08 -0400, Jes Sorensen wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> > On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
>> >> NACK!
>> > SHOUTING!
>> It's normal to write NACK in upper case
>
> Not with an exclamation point.

I would add a <blink> tag if ascii email allowed that!

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Gujulan Elango, Hari Prasath (H.)]

On Thu, May 07, 2015 at 10:09:59AM -0400, Jes Sorensen wrote:
> "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> > On Thu, May 07, 2015 at 03:19:34PM +0300, Dan Carpenter wrote:
> >> On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
> >> > "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> >> > > Remove the goto and return error directly thereby removing a variable
> >> > >
> >> > > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> >> > > ---
> >> > >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
> >> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> >> > 
> >> > NACK!
> >> 
> >> SHOUTING!
> >> 
> >> > 
> >> > The goto is cleaner and more consistent, rather than hiding a return in
> >> > the middle of the code.
> >> 
> >> More consistent with what?  There are already returns earlier in the
> >> function.  Do-nothing gotos are pointless and annoying.
> >> 
> >> regards,
> >> dan carpenter
> >> 
> >
> > I sent this patch  because goto & return was mixed up in this function.
> > There was a return as well as goto used. And i guess goto is not 
> > encouraged anywhere. In this case especially,I couldn't see any 
> > significant cleanup being done in the goto label.
> 
> I will accept a patch to remove the returns in the middle of the
> function, if you wish to submit that.
> 
> A return at the very beginning of a function is OK, but in the middle of
> a larger function they make it harder to catch when code exists.
> 
> Jes

Is this accepted as it is or should I send v2 with the goto retained but
the return in the middle of the function also converted to goto for
consistency.I agree that goto's are used at many places in the kernel
but this is really a small function where it doesn't seem to help
much.Also there is no signficant cleanup being done in the definition of
the label.

regards
Hari

Re: [PATCH] staging: rtl8723au : remove goto & return error directly

[Jes Sorensen]

"Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
> On Thu, May 07, 2015 at 10:09:59AM -0400, Jes Sorensen wrote:
>> "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
>> > On Thu, May 07, 2015 at 03:19:34PM +0300, Dan Carpenter wrote:
>> >> On Thu, May 07, 2015 at 08:03:06AM -0400, Jes Sorensen wrote:
>> >> > "Gujulan Elango, Hari Prasath (H.)" <hgujulan@visteon.com> writes:
>> >> > > Remove the goto and return error directly thereby removing a variable
>> >> > >
>> >> > > Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>> >> > > ---
>> >> > >  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 7 ++-----
>> >> > >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >> > 
>> >> > NACK!
>> >> 
>> >> SHOUTING!
>> >> 
>> >> > 
>> >> > The goto is cleaner and more consistent, rather than hiding a return in
>> >> > the middle of the code.
>> >> 
>> >> More consistent with what?  There are already returns earlier in the
>> >> function.  Do-nothing gotos are pointless and annoying.
>> >> 
>> >> regards,
>> >> dan carpenter
>> >> 
>> >
>> > I sent this patch  because goto & return was mixed up in this function.
>> > There was a return as well as goto used. And i guess goto is not 
>> > encouraged anywhere. In this case especially,I couldn't see any 
>> > significant cleanup being done in the goto label.
>> 
>> I will accept a patch to remove the returns in the middle of the
>> function, if you wish to submit that.
>> 
>> A return at the very beginning of a function is OK, but in the middle of
>> a larger function they make it harder to catch when code exists.
>> 
>> Jes
>
> Is this accepted as it is or should I send v2 with the goto retained but
> the return in the middle of the function also converted to goto for
> consistency.I agree that goto's are used at many places in the kernel
> but this is really a small function where it doesn't seem to help
> much.Also there is no signficant cleanup being done in the definition of
> the label.

As I stated previously, this is not accepted.

Jes