Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: core: Refactor nested if-else

Praveen Kumar
2021-10-25T09:13:10Z

On 25-10-2021 12:55, Kushal Kothari wrote:
> Refactor nested if else by combining nested if into a single if condition and removing unnecessary else conditionals which leads to removing unnecessary tabs .There is no change in logic of new code.
> checkpatch warning : Too many leading tabs - consider code refactoring
>
> Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com>
> ---
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 65 ++++++++-----------
> 1 file changed, 26 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 0f82f5031c43..eb10b6f85426 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1192,46 +1192,33 @@ 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)) {
> + 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;
> +
> + pstat->uapsd_vo = 0;
> + if (pstat->qos_info&0xf) {
> + 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;
> }

there is a bug here, if *p* is not null, and *memcmp* failed; then we fall in else part and break, and will miss the next entry in *p* using below statement

p = p + ie_len + 2;

>

Regards,

~Praveen.