staging: rtl8192u: cleanup of ternary and if else statements

PATCH In review Rebecca Mckeever 5 participants 12 messages Apr 8, 2022 Staging

Share & tools

Rebecca Mckeever

author

2022-04-08 01:12 UTC #0 lore raw

v2 cover staging: rtl8192u: cleanup of ternary and if else statements

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(-)

Signature · 1 line

2.32.0

Rebecca Mckeever

author

2022-04-08 01:13 UTC #1 lore raw

PATCH v2 1/2 staging: rtl8192u: replace ternary statement with if and assignment

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(-)

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1

index 78cc8f357bbc..9885917b9199 100644
@@ -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;

Signature · 1 line

2.32.0

Rebecca Mckeever

author

2022-04-08 01:13 UTC #10 lore raw

PATCH v2 2/2 staging: rtl8192u: use min_t/max_t macros instead of if else

In reply to Rebecca Mckeever

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(-)

drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c +5 -8

index dba3f2db9f48..358c35d9589c 100644
@@ -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

Signature · 1 line

2.32.0

Julia Lawall

2022-04-08 04:15 UTC #2 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

In reply to Rebecca Mckeever

Quoted 14 lines · Rebecca Mckeever

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(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0
index 78cc8f357bbc..9885917b9199 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475

Quoted 7 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1

@@ -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

Quoted 8 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -1
 	erq->flags |= IW_ENCODE_ENABLED;
 
--
2.32.0

Greg Kroah-Hartman

maintainer

2022-04-08 05:09 UTC #3 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

Quoted 13 lines · Julia Lawall

Quoted 9 lines · Rebecca Mckeever
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(-)

Quoted 14 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 12 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
index 78cc8f357bbc..9885917b9199 100644
@@ -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 :)

Dan Carpenter

2022-04-08 05:58 UTC #4 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

In reply to Julia Lawall

Quoted 11 lines · Julia Lawall

Quoted 9 lines · Rebecca Mckeever
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(-)

Quoted 14 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 12 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
index 78cc8f357bbc..9885917b9199 100644
@@ -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

Joe Perches

2022-04-08 06:15 UTC #5 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

Quoted 6 lines · Dan Carpenter

Quoted 5 lines · Julia Lawall
Quoted 3 lines · Rebecca Mckeever
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.

[]

Quoted 1 line

Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0

[]

Quoted 11 lines

Quoted 10 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 8 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
@@ -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?

Quoted 3 lines

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.

Dan Carpenter

2022-04-08 06:32 UTC #6 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

Quoted 21 lines · Joe Perches

Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Julia Lawall
Quoted 3 lines · Rebecca Mckeever
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.
[]
Quoted 1 line
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0
[]
Quoted 11 lines
Quoted 10 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 8 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
@@ -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?

Quoted 7 lines

Quoted 3 lines
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

Dan Carpenter

Reviewed-by on the v2 series

2022-04-08 06:39 UTC #11 lore raw

In reply to Rebecca Mckeever

Quoted 9 lines · Rebecca Mckeever

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

Quoted 4 lines

* 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

Joe Perches

2022-04-08 07:19 UTC #7 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

In reply to Dan Carpenter

Quoted 26 lines · Dan Carpenter

Quoted 25 lines · Joe Perches
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Julia Lawall
Quoted 3 lines · Rebecca Mckeever
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.
[]
Quoted 1 line
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0
[]
Quoted 14 lines
Quoted 10 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 8 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
@@ -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.

Quoted 6 lines

Quoted 3 lines
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.

Dan Carpenter

2022-04-08 08:36 UTC #8 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

Quoted 31 lines · Joe Perches

Quoted 30 lines · Dan Carpenter
Quoted 28 lines · Joe Perches
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Julia Lawall
Quoted 3 lines · Rebecca Mckeever
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.
[]
Quoted 1 line
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
Quoted 1 line
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0
[]
Quoted 11 lines
Quoted 10 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 8 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
@@ -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?
Quoted 3 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

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

Julia Lawall

2022-04-08 08:51 UTC #9 lore raw

Re: [PATCH v2 1/2] staging: rtl8192u: replace ternary statement with if and assignment

In reply to Dan Carpenter

Quoted 27 lines · Dan Carpenter

Quoted 11 lines · Julia Lawall
Quoted 9 lines · Rebecca Mckeever
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(-)
Quoted 14 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 475
Quoted 12 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +3 -1
index 78cc8f357bbc..9885917b9199 100644
@@ -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?

Quoted 3 lines

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