| 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
| ||
|
Message-ID: <20220329212426.GA1171538@alison-desk>
Date: Tue, 29 Mar 2022 14:24:26 -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.
On Tue, Mar 29, 2022 at 01:59:52PM -0700, Sevinj Aghayeva wrote:
> Hi Alison,
>
> Thanks for the feedback! Please see inline.
>
> On Tue, Mar 29, 2022 at 12:58 PM Alison Schofield <
> alison.schofield@...el.com> wrote:
>
> > 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.
> >
>
> Got it. The few examples that I saw in other places were using
> grammatically formatted sentences, but I'll take the subsystem conventions
> into account.
>
>
> > 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
>
>
> My bad! I ran get_maintainer.pl without those options and got those a lot
> more email addresses.
>
>
> >
> >
> >
> > 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:
> >
>
> Okay. I think we should update the https://kernelnewbies.org/PatchPhilosophy
> because I took "This patch..." from there.
Yeah, there's a lot of 'This patch...' and 'Fixes...' and some other
stuff that has fallen out of favor.
I just made a small edit getting rid of a 'Fixes' subject line.
How about if you update this example:
Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style
with your patch right here..once it is done!
Thanks!
Alison
>
>
> >
> > You have another patch in flight with similar things to update.
> > Please post a v2 of that one that addresses the feedback given
> > here.
> >
>
> That one has already landed. Is it okay if I send v2 just for this patch?
Did you see it applied already?
>
> Thanks!
>
>
> >
> > 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
> > >
> > >
> >
>
>
> --
>
> Sevinj.Aghayeva
Powered by blists - more mailing lists