| 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: <eb7db8fa-0557-425f-b08d-1d55ac07610a@redhat.com>
Date: Tue, 26 Oct 2021 17:15:13 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: kushal kothari <kushalkothari285@...il.com>,
Mike Rapoport <mike.rapoport@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
fabioaiuto83@...il.com, ross.schm.dev@...il.com,
marcocesati@...il.com,
"Fabio M. De Francesco" <fmdefrancesco@...il.com>,
linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
outreachy-kernel@...glegroups.com, kushalkothari2850@...il.com
Subject: Re: [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
Hi,
On 10/26/21 17:12, kushal kothari wrote:
> Yes!
> Also initializing variable pstat->has_legacy_ac = false; //false
> which is previously set to true before the loop so it can be removed too.
Note please do not make too many changes in a single patch.
It is best to do refactoring like this in small steps making
only 1 change at a time, that makes things a lot easier to
review / to verify that there are no functional changes.
Regards,
Hans
>
> On Tue, Oct 26, 2021 at 7:49 PM Mike Rapoport <mike.rapoport@...il.com <mailto:mike.rapoport@...il.com>> wrote:
>
> On Tue, Oct 26, 2021 at 07:12:53PM +0530, Kushal Kothari wrote:
> > Refactor nested if-else to avoid deep indentations. There is no change
> > in the logic of the new code, however, now it is simple because it gets
> > rid of five unnecessary else conditionals and it combines nested if into
> > single if-else-if. This refactor also leads to fix warning detected by
> > checkpatch.pl <http://checkpatch.pl>:
> > WARNING: Too many leading tabs - consider code refactoring
> >
> > Signed-off-by: Kushal Kothari <kushalkothari285@...il.com <mailto:kushalkothari285@...il.com>>
> > ---
> >
> > Changes in v2: Fix the bug of not handling properly the else logic
> > when p is not null in else-if. Also, reword the subject line and break
> > it up at 72 columns.
> >
> > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
> > 1 file changed, 29 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > index 0f82f5031c43..267d853b1514 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > @@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
> > p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
> > for (;;) {
> > p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> > - if (p) {
> > - if (!memcmp(p+2, WMM_IE, 6)) {
> > -
> > - pstat->flags |= WLAN_STA_WME;
> > -
> > - pstat->qos_option = 1;
> > - pstat->qos_info = *(p+8);
> > -
> > - pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > -
> > - if ((pstat->qos_info&0xf) != 0xf)
> > - pstat->has_legacy_ac = true;
> > - else
> > - pstat->has_legacy_ac = false;
> > -
> > - if (pstat->qos_info&0xf) {
> > - if (pstat->qos_info&BIT(0))
> > - pstat->uapsd_vo = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_vo = 0;
> > -
> > - if (pstat->qos_info&BIT(1))
> > - pstat->uapsd_vi = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_vi = 0;
> > -
> > - if (pstat->qos_info&BIT(2))
> > - pstat->uapsd_bk = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_bk = 0;
> > -
> > - if (pstat->qos_info&BIT(3))
> > - pstat->uapsd_be = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_be = 0;
> > -
> > - }
> > -
> > - break;
> > + if (p && memcmp(p+2, WMM_IE, 6)) {
> > + p = p + ie_len + 2;
> > + } else if (p && !memcmp(p+2, WMM_IE, 6)) {
> > + pstat->flags |= WLAN_STA_WME;
> > + pstat->qos_option = 1;
> > + pstat->qos_info = *(p+8);
> > + pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > +
> > + pstat->has_legacy_ac = false;
> > + if ((pstat->qos_info&0xf) != 0xf)
> > + pstat->has_legacy_ac = true;
> > +
> > + if (pstat->qos_info&0xf) {
> > + pstat->uapsd_vo = 0;
>
> This variable and other variables below are set to 0 just before the loop,
> so the initialization here can be removed.
>
> > + if (pstat->qos_info&BIT(0))
> > + pstat->uapsd_vo = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_vi = 0;
> > + if (pstat->qos_info&BIT(1))
> > + pstat->uapsd_vi = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_bk = 0;
> > + if (pstat->qos_info&BIT(2))
> > + pstat->uapsd_bk = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_be = 0;
> > + if (pstat->qos_info&BIT(3))
> > + pstat->uapsd_be = BIT(0)|BIT(1);
> > }
> > + break;
> > } else {
> > break;
> > }
> > - p = p + ie_len + 2;
> > }
> > }
> >
> > --
> > 2.25.1
> >
>
> --
> Sincerely yours,
> Mike.
>
Powered by blists - more mailing lists