[PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Chris Opperman]

Improve readability of prism2_add_key:
a) Reduce nesting and removed goto statement by using more return statements.
b) Added temporary "key" variable to reduce line length.

Signed-off-by: Chris Opperman <eklikeroomys@gmail.com>
---

Improve readability of prism2_add_key:
a) Reduce nesting and removed goto statement by using more return statements.
b) Added temporary "key" variable to reduce line length.

 drivers/staging/wlan-ng/cfg80211.c | 40 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
index 4291225..b46db65 100644
--- a/drivers/staging/wlan-ng/cfg80211.c
+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -147,41 +147,27 @@ static int prism2_add_key(struct wiphy *wiphy, struct net_device *dev,
 {
 	struct wlandevice *wlandev = dev->ml_priv;
 	u32 did;
-
-	int err = 0;
-	int result = 0;
+	u32 key;
 
 	if (key_index >= NUM_WEPKEYS)
 		return -EINVAL;
 
-	switch (params->cipher) {
-	case WLAN_CIPHER_SUITE_WEP40:
-	case WLAN_CIPHER_SUITE_WEP104:
-		result = prism2_domibset_uint32(wlandev,
-						DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
-						key_index);
-		if (result)
-			goto exit;
-
-		/* send key to driver */
-		did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_key(key_index + 1);
-
-		result = prism2_domibset_pstr32(wlandev, did,
-						params->key_len, params->key);
-		if (result)
-			goto exit;
-		break;
-
-	default:
+	if (params->cipher != WLAN_CIPHER_SUITE_WEP40 &&
+	    params->cipher != WLAN_CIPHER_SUITE_WEP104) {
 		pr_debug("Unsupported cipher suite\n");
-		result = 1;
+		return -EFAULT;
 	}
 
-exit:
-	if (result)
-		err = -EFAULT;
+	key = DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID;
+	if (prism2_domibset_uint32(wlandev, key, key_index))
+		return -EFAULT;
 
-	return err;
+	/* send key to driver */
+	did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_key(key_index + 1);
+
+	if (prism2_domibset_pstr32(wlandev, did, params->key_len, params->key))
+		return -EFAULT;
+	return 0;
 }
 
 static int prism2_get_key(struct wiphy *wiphy, struct net_device *dev,
-- 
2.1.4

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Dan Carpenter]

On Wed, Jun 20, 2018 at 07:10:26PM +0200, Chris Opperman wrote:
> b) Added temporary "key" variable to reduce line length.

No, that's nonsense.  Just fix the original variable so that it's not a
million characters long.

Using a shorter variable is good if it simplifies the code like so:

	struct foo *foo = &my->random_variable[INDEX].whatever;

You've reduced 4 variable names and a butt load of punctuation down to
just "foo" so that we don't have to look at it repeated over and over
on every line.

But if you're just using a temporary variable because the original
variable name was poorly chosen, that's not the right thing.

regards,
dan carpenter

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Chris Opperman]

Hi Dan,

I agree completely. I was concerned whether the preprocessor definitions in
p80211metadef.h were named according to some convention as there are
many definitions named similarly there. 

I am considering renaming this specific definition to "p80211_dot11_keyid", 
would that be more acceptable?

Kind Regards,
Chris Opperman

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Dan Carpenter]

On Thu, Jun 21, 2018 at 06:42:52AM +0200, Chris Opperman wrote:
> Hi Dan,
> 
> I agree completely. I was concerned whether the preprocessor definitions in
> p80211metadef.h were named according to some convention as there are
> many definitions named similarly there.
> 

Yeah, sure but in that case leave this code alone until we get the time
to update all the definitions.

> I am considering renaming this specific definition to "p80211_dot11_keyid",
> would that be more acceptable?

There is probably a normal define that wireless drivers already use, but
I don't know networking enough to say what it is...  You should try
asking on linux-wireless.

It says in the header that the name is generated automatically?  Is that
true?  How does that work?

Probably it's better to make it upper case?  Are both the P80211 and the
DOT11 required or is DOT11 a subset?  Anyway, I don't think it's
possible to create a worse naming scheme than the driver already has so
I'm likely to approve anything you send.

regards,
dan carpenter

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Chris Opperman]

Hi Dan,
The header also states "DO NOT EDIT OR MODIFY". As you suggested, I
will rather leave this patch for now.

P.S. Please advise if there is anything specific I can help out with, or I'll 
keep looking for more obvious fixes I can make while I'm learning the
ropes.
Kind Regards,
Chris Opperman

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Dan Carpenter]

I liked the patch, but just go over the 80 character limit instead of
adding a "key" variable.  It's better that it generate a checkpatch
warning and forces someone to rename the variable...

regards,
dan carpenter

Re: [PATCH] staging: wlan-ng: improved readability of function prism2_add_key

[Chris Opperman]

Okay, in that case I will fix and resend the patch.

Kind Regards,
Chris Opperman