These patches replace ternary and if else statements with
more readable statements. Found with minmax coccinelle script.
---
v1 -> v2:
* Patch 1: "staging: rtl8192u: replace ternary statement with if and
* assignment"
* replaced max macro with an if statement followed by an assignment
* Patch 2: "staging: rtl8192u: use min_t/max_t macros instead of if
* else"
* changed the type argument in min_t and max_t from u8 to u32
---
Rebecca Mckeever (2):
staging: rtl8192u: replace ternary statement with if and assignment
staging: rtl8192u: use min_t/max_t macros instead of if else
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
2 files changed, 8 insertions(+), 9 deletions(-)
--
2.32.0
Replace ternary statement with an if statement followed by an assignment
to increase readability and make error handling more obvious.
Found with minmax coccinelle script.
Signed-off-by: Rebecca Mckeever <[email protected]>
---
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
index 78cc8f357bbc..9885917b9199 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
@@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
return 0;
}
len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
- erq->length = (len >= 0 ? len : 0);
+ if (len < 0)
+ len = 0;
+ erq->length = len;
erq->flags |= IW_ENCODE_ENABLED;
--
2.32.0
Replace if else statement with min_t or max_t macros to increase
readability and conform to Linux kernel coding style. The _t versions
of the macros must be used to avoid applying typeof to the bit fields
pPeerHTCap->MaxRxAMPDUFactor, and pPeerHTCap->MPDUDensity.
Using u32 assures the reader that the value with not be truncated
without having to look up the types of the variables involved.
Found with minmax coccinelle script.
Signed-off-by: Rebecca Mckeever <[email protected]>
---
drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
index dba3f2db9f48..358c35d9589c 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
@@ -940,10 +940,8 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
else
pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K;
} else {
- if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K)
- pHTInfo->CurrentAMPDUFactor = pPeerHTCap->MaxRxAMPDUFactor;
- else
- pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K;
+ pHTInfo->CurrentAMPDUFactor = min_t(u32, pPeerHTCap->MaxRxAMPDUFactor,
+ HT_AGG_SIZE_32K);
}
}
@@ -951,10 +949,9 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
* <2> Set AMPDU Minimum MPDU Start Spacing
* 802.11n 3.0 section 9.7d.3
*/
- if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity)
- pHTInfo->CurrentMPDUDensity = pHTInfo->MPDU_Density;
- else
- pHTInfo->CurrentMPDUDensity = pPeerHTCap->MPDUDensity;
+ pHTInfo->CurrentMPDUDensity = max_t(u32, pHTInfo->MPDU_Density,
+ pPeerHTCap->MPDUDensity);
+
if (ieee->pairwise_key_type != KEY_TYPE_NA)
pHTInfo->CurrentMPDUDensity = 7; // 8us
// Force TX AMSDU
--
2.32.0
On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> Replace ternary statement with an if statement followed by an assignment
> to increase readability and make error handling more obvious.
> Found with minmax coccinelle script.
>
> Signed-off-by: Rebecca Mckeever <[email protected]>
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> index 78cc8f357bbc..9885917b9199 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> return 0;
> }
> len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> - erq->length = (len >= 0 ? len : 0);
> + if (len < 0)
> + len = 0;
> + erq->length = len;
Maybe you could use max here?
julia
>
> erq->flags |= IW_ENCODE_ENABLED;
>
> --
> 2.32.0
>
>
>
On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
>
>
> On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
>
> > Replace ternary statement with an if statement followed by an assignment
> > to increase readability and make error handling more obvious.
> > Found with minmax coccinelle script.
> >
> > Signed-off-by: Rebecca Mckeever <[email protected]>
> > ---
> > drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > index 78cc8f357bbc..9885917b9199 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > return 0;
> > }
> > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > - erq->length = (len >= 0 ? len : 0);
> > + if (len < 0)
> > + len = 0;
> > + erq->length = len;
>
> Maybe you could use max here?
Dan said to not do this on based on the first submission :)
On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
>
> > Replace ternary statement with an if statement followed by an assignment
> > to increase readability and make error handling more obvious.
> > Found with minmax coccinelle script.
> >
> > Signed-off-by: Rebecca Mckeever <[email protected]>
> > ---
> > drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > index 78cc8f357bbc..9885917b9199 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > return 0;
> > }
> > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > - erq->length = (len >= 0 ? len : 0);
> > + if (len < 0)
> > + len = 0;
> > + erq->length = len;
>
> Maybe you could use max here?
Initially Rebecca did use max() but I NAKed it. It's really not less
readable. Better to handle the error explicitly. Keep the error path
indented two tabs. Separate from the success path.
regards,
dan carpenter
On Fri, 2022-04-08 at 08:57 +0300, Dan Carpenter wrote:
> On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> > On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> >
> > > Replace ternary statement with an if statement followed by an assignment
> > > to increase readability and make error handling more obvious.
> > > Found with minmax coccinelle script.
[]
> > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
[]
> > > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > > return 0;
> > > }
> > > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > > - erq->length = (len >= 0 ? len : 0);
> > > + if (len < 0)
> > > + len = 0;
> > > + erq->length = len;
> >
> > Maybe you could use max here?
>
> Initially Rebecca did use max() but I NAKed it. It's really not less
> readable. Better to handle the error explicitly. Keep the error path
> indented two tabs. Separate from the success path.
A comment would be useful as it's not obvious it's an 'error' path.
One has to read all 3 get_key functions to determine that.
On Thu, Apr 07, 2022 at 11:14:51PM -0700, Joe Perches wrote:
> On Fri, 2022-04-08 at 08:57 +0300, Dan Carpenter wrote:
> > On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> > > On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> > >
> > > > Replace ternary statement with an if statement followed by an assignment
> > > > to increase readability and make error handling more obvious.
> > > > Found with minmax coccinelle script.
> []
> > > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> []
> > > > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > > > return 0;
> > > > }
> > > > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > > > - erq->length = (len >= 0 ? len : 0);
> > > > + if (len < 0)
> > > > + len = 0;
> > > > + erq->length = len;
> > >
> > > Maybe you could use max here?
> >
> > Initially Rebecca did use max() but I NAKed it. It's really not less
> > readable. Better to handle the error explicitly. Keep the error path
> > indented two tabs. Separate from the success path.
>
> A comment would be useful as it's not obvious it's an 'error' path.
> One has to read all 3 get_key functions to determine that.
>
I'm so confused. Negative error codes are the common case in the
kernel. We don't need to comment it.
Obviously in an ideal world the get_key() functions would return -EINVAL
instead of -1...
regards,
dan carpenter
On Thu, Apr 07, 2022 at 08:12:49PM -0500, Rebecca Mckeever wrote:
> These patches replace ternary and if else statements with
> more readable statements. Found with minmax coccinelle script.
>
> ---
> v1 -> v2:
> * Patch 1: "staging: rtl8192u: replace ternary statement with if and
> * assignment"
> * replaced max macro with an if statement followed by an assignment
>
> * Patch 2: "staging: rtl8192u: use min_t/max_t macros instead of if
> * else"
> * changed the type argument in min_t and max_t from u8 to u32
> ---
Looks great. Thanks!
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
On Fri, 2022-04-08 at 09:31 +0300, Dan Carpenter wrote:
> On Thu, Apr 07, 2022 at 11:14:51PM -0700, Joe Perches wrote:
> > On Fri, 2022-04-08 at 08:57 +0300, Dan Carpenter wrote:
> > > On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> > > > On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> > > >
> > > > > Replace ternary statement with an if statement followed by an assignment
> > > > > to increase readability and make error handling more obvious.
> > > > > Found with minmax coccinelle script.
> > []
> > > > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > []
> > > > > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > > > > return 0;
> > > > > }
> > > > > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > > > > - erq->length = (len >= 0 ? len : 0);
> > > > > + if (len < 0)
> > > > > + len = 0;
> > > > > + erq->length = len;
> > > >
> > > > Maybe you could use max here?
> > >
> > > Initially Rebecca did use max() but I NAKed it. It's really not less
> > > readable. Better to handle the error explicitly. Keep the error path
> > > indented two tabs. Separate from the success path.
> >
> > A comment would be useful as it's not obvious it's an 'error' path.
> > One has to read all 3 get_key functions to determine that.
> >
>
> I'm so confused. Negative error codes are the common case in the
> kernel. We don't need to comment it.
If it was an error, it would handle it as an error not set
len to 0 and continue. That's why IMO a comment is useful.
On Fri, Apr 08, 2022 at 12:19:01AM -0700, Joe Perches wrote:
> On Fri, 2022-04-08 at 09:31 +0300, Dan Carpenter wrote:
> > On Thu, Apr 07, 2022 at 11:14:51PM -0700, Joe Perches wrote:
> > > On Fri, 2022-04-08 at 08:57 +0300, Dan Carpenter wrote:
> > > > On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> > > > > On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> > > > >
> > > > > > Replace ternary statement with an if statement followed by an assignment
> > > > > > to increase readability and make error handling more obvious.
> > > > > > Found with minmax coccinelle script.
> > > []
> > > > > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > > []
> > > > > > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > > > > > return 0;
> > > > > > }
> > > > > > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > > > > > - erq->length = (len >= 0 ? len : 0);
> > > > > > + if (len < 0)
> > > > > > + len = 0;
> > > > > > + erq->length = len;
> > > > >
> > > > > Maybe you could use max here?
> > > >
> > > > Initially Rebecca did use max() but I NAKed it. It's really not less
> > > > readable. Better to handle the error explicitly. Keep the error path
> > > > indented two tabs. Separate from the success path.
> > >
> > > A comment would be useful as it's not obvious it's an 'error' path.
> > > One has to read all 3 get_key functions to determine that.
> > >
> >
> > I'm so confused. Negative error codes are the common case in the
> > kernel. We don't need to comment it.
>
> If it was an error, it would handle it as an error not set
> len to 0 and continue. That's why IMO a comment is useful.
Yeah. You're probably right. My understanding is that a zero length
key is a special case where it uses the default key? Which I guess is
all zeroes here.
if (len < 0) {
/* No key data. Use the default key. */
len = 0;
}
But when I look at this some more then there are three ->get_key()
callers in this file and only this one checks for -1 returns. For the
one caller that does this:
ext->key_len = crypt->ops->get_key(ext->key, SCM_KEY_LEN, NULL, crypt->priv);
then a negative return would result in a buffer overflow.
So another option would be to just return 0 instead of -1 from the
get_key() functions.
File | Pointer | Function | Static
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | (struct ieee80211_crypto_ops)->get_key | ieee80211_tkip_get_key | 1
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c | (struct ieee80211_crypto_ops)->get_key | prism2_wep_get_key | 1
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | (struct ieee80211_crypto_ops)->get_key | ieee80211_ccmp_get_key | 1
Changing it to return zero would leave ieee80211_wx_get_encode() behavior
as-is. It would fix a buffer overflow in ieee80211_wx_get_encode_ext().
It is a behavior change in ieee80211_wx_set_encode() and I think that's
a bug fix as well.
regards,
dan carpenter
On Fri, 8 Apr 2022, Dan Carpenter wrote:
> On Fri, Apr 08, 2022 at 06:15:14AM +0200, Julia Lawall wrote:
> > On Thu, 7 Apr 2022, Rebecca Mckeever wrote:
> >
> > > Replace ternary statement with an if statement followed by an assignment
> > > to increase readability and make error handling more obvious.
> > > Found with minmax coccinelle script.
> > >
> > > Signed-off-by: Rebecca Mckeever <[email protected]>
> > > ---
> > > drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > > index 78cc8f357bbc..9885917b9199 100644
> > > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > > @@ -470,7 +470,9 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
> > > return 0;
> > > }
> > > len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> > > - erq->length = (len >= 0 ? len : 0);
> > > + if (len < 0)
> > > + len = 0;
> > > + erq->length = len;
> >
> > Maybe you could use max here?
>
> Initially Rebecca did use max() but I NAKed it. It's really not less
> readable. Better to handle the error explicitly. Keep the error path
> indented two tabs. Separate from the success path.
OK. I have a hard time seeing that as an error path, though, with no
return or goto. I guess it's error and recover. OK either way.
julia