linux-kernel - Re: [PATCH] staging: rtl8723bs: Remove redundant else branches.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 29 Mar 2022 13:00:53 -0700
From:   Alison Schofield <alison.schofield@...el.com>
To:     Sevinj Aghayeva <sevinj.aghayeva@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Fabio Aiuto <fabioaiuto83@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Muhammad Usama Anjum <musamaanjum@...il.com>,
        "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev
Subject: Re: [PATCH] staging: rtl8723bs: Remove redundant else branches.

Hi Sevinj,

You can do this to make sure your commit msg 'looks' similar to
others in the file:

$git log --pretty=oneline --abbrev-commit

d6ad258ae15d staging: rtl8723bs: Remove redundant else branches.
24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch)
167fc30e8e51 staging: rtl8723bs: remove unused macros
d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info
6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid

That tells me the 'Remove' should not start with an uppercase and
there should be no period at the end of line. Note that I look at this
per patch, because different drivers and subsystems have different
conventions. The point is to try to discern the prevailing style,
and follow it.

Please check the get_maintainers instruction in the first patch tutorial.
I only got this: 

$ git show HEAD | perl scripts/get_maintainer.pl --separator , --nokeywords --nogit --nogit-fallback --norolestats
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,linux-staging@...ts.linux.dev,linux-kernel@...r.kernel.org


On Tue, Mar 29, 2022 at 07:09:04AM -0700, Sevinj Aghayeva wrote:

Please state the 'why' of this patch. ie something like:
Adhere to Linux kernel coding style.

> This patch fixes the following checkpatch.pl warning:
The phrases 'This patch' is frowned upon as unecessarily wordy.
Perhaps simply:
Reported by checkpatch:

You have another patch in flight with similar things to update.
Please post a v2 of that one that addresses the feedback given
here.

I thought both patches had good scope in that they addressed multiple
instances of the same issue in a single file.

Thanks!
Alison

> 
> WARNING: else is not generally useful after a break or return
> 
> Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva@...il.com>
> ---
>  .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index b449be537376..27de086903e2 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
>  
>  int rtw_check_network_type(unsigned char *rate, int ratelen, int channel)
>  {
> -	if (channel > 14) {
> +	if (channel > 14)
>  		return WIRELESS_INVALID;
> -	} else { /*  could be pure B, pure G, or B/G */
> -		if (rtw_is_cckratesonly_included(rate))
> -			return WIRELESS_11B;
> -		else if (rtw_is_cckrates_included(rate))
> -			return	WIRELESS_11BG;
> -		else
> -			return WIRELESS_11G;
> -	}
> +	/* could be pure B, pure G, or B/G */
> +	if (rtw_is_cckratesonly_included(rate))
> +		return WIRELESS_11B;
> +	if (rtw_is_cckrates_included(rate))
> +		return WIRELESS_11BG;
> +	return WIRELESS_11G;
>  }
>  
>  u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned char *source,
> @@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed int *len, signed int limit)
>  		if (*p == index) {
>  			*len = *(p + 1);
>  			return p;
> -		} else {
> -			tmp = *(p + 1);
> -			p += (tmp + 2);
> -			i += (tmp + 2);
>  		}
> +		tmp = *(p + 1);
> +		p += (tmp + 2);
> +		i += (tmp + 2);
>  		if (i >= limit)
>  			break;
>  	}
> @@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8 *oui, u8 oui_len, u8 *ie, u
>  				*ielen = in_ie[cnt+1]+2;
>  
>  			break;
> -		} else {
> -			cnt += in_ie[cnt+1]+2; /* goto next */
>  		}
> +		cnt += in_ie[cnt+1]+2; /* goto next */
>  	}
>  
>  	return target_ie;
> @@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
>  			cnt += in_ie[cnt+1]+2;
>  
>  			break;
> -		} else {
> -			cnt += in_ie[cnt+1]+2; /* goto next */
>  		}
> +		cnt += in_ie[cnt+1]+2; /* goto next */
>  	}
>  
>  	return wpsie_ptr;
> @@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
>  				*len_attr = attr_len;
>  
>  			break;
> -		} else {
> -			attr_ptr += attr_len; /* goto next */
>  		}
> +		attr_ptr += attr_len; /* goto next */
>  	}
>  
>  	return target_attr_ptr;
> -- 
> 2.25.1
> 
>