* [PATCH 0/2] staging: r8188eu: more error code cleanups
@ 2022-07-28 23:11 Phillip Potter
2022-07-28 23:11 ` [PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup Phillip Potter
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Phillip Potter @ 2022-07-28 23:11 UTC (permalink / raw)
To: gregkh
Cc: Larry.Finger, dan.carpenter, paskripkin, martin, straube.linux,
fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel
This small series addresses a cleanup suggestion discussed by Greg
Kroah-Hartman and Dan Carpenter, and also includes another function
conversion. It would be a larger series, but my time being what it is,
I intend to chip away at these slowly and steadily.
Phillip Potter (2):
staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup
staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics
drivers/staging/r8188eu/core/rtw_ioctl_set.c | 8 ++++----
drivers/staging/r8188eu/core/rtw_pwrctrl.c | 2 +-
drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +-
drivers/staging/r8188eu/os_dep/ioctl_linux.c | 5 ++---
4 files changed, 8 insertions(+), 9 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup 2022-07-28 23:11 [PATCH 0/2] staging: r8188eu: more error code cleanups Phillip Potter @ 2022-07-28 23:11 ` Phillip Potter 2022-07-28 23:11 ` [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics Phillip Potter 2022-07-29 6:09 ` [PATCH 0/2] staging: r8188eu: more error code cleanups Philipp Hortmann 2 siblings, 0 replies; 12+ messages in thread From: Phillip Potter @ 2022-07-28 23:11 UTC (permalink / raw) To: gregkh Cc: Larry.Finger, dan.carpenter, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel Remove the success initializer from the ret variable in rtw_pwr_wakeup, as we set it later anyway in the success path, and also set on failure. This makes the function appear cleaner and more consistent. Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- drivers/staging/r8188eu/core/rtw_pwrctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c index 8b1c50668dfe..75e655bae16a 100644 --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c @@ -381,7 +381,7 @@ int rtw_pwr_wakeup(struct adapter *padapter) struct mlme_priv *pmlmepriv = &padapter->mlmepriv; unsigned long timeout = jiffies + msecs_to_jiffies(3000); unsigned long deny_time; - int ret = 0; + int ret; while (pwrpriv->ps_processing && time_before(jiffies, timeout)) msleep(10); -- 2.36.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-28 23:11 [PATCH 0/2] staging: r8188eu: more error code cleanups Phillip Potter 2022-07-28 23:11 ` [PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup Phillip Potter @ 2022-07-28 23:11 ` Phillip Potter 2022-07-29 6:48 ` Dan Carpenter 2022-08-02 20:10 ` Pavel Skripkin 2022-07-29 6:09 ` [PATCH 0/2] staging: r8188eu: more error code cleanups Philipp Hortmann 2 siblings, 2 replies; 12+ messages in thread From: Phillip Potter @ 2022-07-28 23:11 UTC (permalink / raw) To: gregkh Cc: Larry.Finger, dan.carpenter, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel Convert the rtw_set_802_11_add_wep function to use 0 on success and an appropriate error code on error. Also convert return type to int to allow negative return values. For the first failure block where keyid >= 4, use -EOPNOTSUPP as this is most appropriate, and for the second failure block where rtw_set_key is called, use -ENOMEM, as this is the cause of failure in that function anyway - in due course, rtw_set_key can be converted as well. Finally, convert both call-sites of rtw_set_802_11_add_wep to use the new semantics, passing through the error code where we can for one of them. This gets the driver closer to removal of the non-standard _SUCCESS and _FAIL definitions, which are inverted compared to the standard in-kernel error code mechanism. Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- drivers/staging/r8188eu/core/rtw_ioctl_set.c | 8 ++++---- drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +- drivers/staging/r8188eu/os_dep/ioctl_linux.c | 5 ++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c index 17f6bcbeebf4..a5b5d7f6a864 100644 --- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c +++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c @@ -390,16 +390,16 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11 return ret; } -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) { int keyid, res; struct security_priv *psecuritypriv = &padapter->securitypriv; - u8 ret = _SUCCESS; + int ret = 0; keyid = wep->KeyIndex & 0x3fffffff; if (keyid >= 4) { - ret = false; + ret = -EOPNOTSUPP; goto exit; } @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) res = rtw_set_key(padapter, psecuritypriv, keyid, 1); if (res == _FAIL) - ret = false; + ret = -ENOMEM; exit: return ret; diff --git a/drivers/staging/r8188eu/include/rtw_ioctl_set.h b/drivers/staging/r8188eu/include/rtw_ioctl_set.h index 7365079c704f..761b30bdb8ec 100644 --- a/drivers/staging/r8188eu/include/rtw_ioctl_set.h +++ b/drivers/staging/r8188eu/include/rtw_ioctl_set.h @@ -11,7 +11,7 @@ typedef u8 NDIS_802_11_PMKID_VALUE[16]; u8 rtw_set_802_11_authentication_mode(struct adapter *adapt, enum ndis_802_11_auth_mode authmode); u8 rtw_set_802_11_bssid(struct adapter*adapter, u8 *bssid); -u8 rtw_set_802_11_add_wep(struct adapter *adapter, struct ndis_802_11_wep *wep); +int rtw_set_802_11_add_wep(struct adapter *adapter, struct ndis_802_11_wep *wep); u8 rtw_set_802_11_disassociate(struct adapter *adapter); u8 rtw_set_802_11_bssid_list_scan(struct adapter*adapter, struct ndis_802_11_ssid *pssid, diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c index 7f91dac2e41b..4d8dbc2a9ef2 100644 --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c @@ -422,8 +422,7 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param, pwep->KeyIndex |= 0x80000000; memcpy(pwep->KeyMaterial, param->u.crypt.key, pwep->KeyLength); if (param->u.crypt.set_tx) { - if (rtw_set_802_11_add_wep(padapter, pwep) == (u8)_FAIL) - ret = -EOPNOTSUPP; + ret = rtw_set_802_11_add_wep(padapter, pwep); } else { if (wep_key_idx >= WEP_KEYS) { ret = -EOPNOTSUPP; @@ -1613,7 +1612,7 @@ static int rtw_wx_set_enc(struct net_device *dev, memcpy(wep.KeyMaterial, keybuf, wep.KeyLength); - if (!rtw_set_802_11_add_wep(padapter, &wep)) { + if (rtw_set_802_11_add_wep(padapter, &wep)) { if (rf_on == pwrpriv->rf_pwrstate) ret = -EOPNOTSUPP; goto exit; -- 2.36.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-28 23:11 ` [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics Phillip Potter @ 2022-07-29 6:48 ` Dan Carpenter 2022-07-30 18:36 ` Phillip Potter 2022-08-02 20:10 ` Pavel Skripkin 1 sibling, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2022-07-29 6:48 UTC (permalink / raw) To: Phillip Potter Cc: gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote: > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > { > int keyid, res; > struct security_priv *psecuritypriv = &padapter->securitypriv; > - u8 ret = _SUCCESS; > + int ret = 0; > > keyid = wep->KeyIndex & 0x3fffffff; > > if (keyid >= 4) { > - ret = false; > + ret = -EOPNOTSUPP; > goto exit; > } > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > res = rtw_set_key(padapter, psecuritypriv, keyid, 1); > > if (res == _FAIL) > - ret = false; > + ret = -ENOMEM; > exit: > > return ret; No, this isn't right. This now returns 1 on success and negative error codes on error. There are a couple anti-patterns here: 1) Do nothing gotos 2) Mixing error paths and success paths. If you avoid mixing error paths and success paths then you get a pattern called: "Solid return zero." This is where the end of the function has a very chunky "return 0;" to mark that it is successful. You want that. Some people do a "if (ret == 0) return ret;". Nope. "return ret;" is not chunky. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-29 6:48 ` Dan Carpenter @ 2022-07-30 18:36 ` Phillip Potter 2022-07-31 17:12 ` Joe Perches 2022-08-01 7:00 ` Dan Carpenter 0 siblings, 2 replies; 12+ messages in thread From: Phillip Potter @ 2022-07-30 18:36 UTC (permalink / raw) To: Dan Carpenter Cc: gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote: > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote: > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > { > > int keyid, res; > > struct security_priv *psecuritypriv = &padapter->securitypriv; > > - u8 ret = _SUCCESS; > > + int ret = 0; > > > > keyid = wep->KeyIndex & 0x3fffffff; > > > > if (keyid >= 4) { > > - ret = false; > > + ret = -EOPNOTSUPP; > > goto exit; > > } > > > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > res = rtw_set_key(padapter, psecuritypriv, keyid, 1); > > > > if (res == _FAIL) > > - ret = false; > > + ret = -ENOMEM; > > exit: > > > > return ret; > > No, this isn't right. This now returns 1 on success and negative > error codes on error. > > There are a couple anti-patterns here: > > 1) Do nothing gotos > 2) Mixing error paths and success paths. > > If you avoid mixing error paths and success paths then you get a pattern > called: "Solid return zero." This is where the end of the function has > a very chunky "return 0;" to mark that it is successful. You want that. > Some people do a "if (ret == 0) return ret;". Nope. "return ret;" is > not chunky. > > regards, > dan carpenter > Hi Dan, Thank you for the review firstly, much appreciated. I'm happy of course to rewrite this to address any concerns, but I was hoping I could clarify what you've said though? Apologies if I've missed it, but how is this function now returning 1 on success? It sets ret to 0 (success) at the start and then sets it to one of two negative error codes depending on what happens. Am I missing something here? (Perfectly possible that I am). In terms of do nothing gotos, do you mean gotos that just set an error code then jump to the end? If you'd prefer, as the function just returns right after the exit label, I can just return the codes directly and have a 'return 0;' like you say above? Thanks as always for your insight. Regards, Phil ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-30 18:36 ` Phillip Potter @ 2022-07-31 17:12 ` Joe Perches 2022-08-01 9:17 ` Dan Carpenter 2022-08-02 23:26 ` Phillip Potter 2022-08-01 7:00 ` Dan Carpenter 1 sibling, 2 replies; 12+ messages in thread From: Joe Perches @ 2022-07-31 17:12 UTC (permalink / raw) To: Phillip Potter, Dan Carpenter Cc: gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Sat, 2022-07-30 at 19:36 +0100, Phillip Potter wrote: > On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote: > > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote: > > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > { > > > int keyid, res; > > > struct security_priv *psecuritypriv = &padapter->securitypriv; > > > - u8 ret = _SUCCESS; > > > + int ret = 0; > > > > > > keyid = wep->KeyIndex & 0x3fffffff; > > > > > > if (keyid >= 4) { > > > - ret = false; > > > + ret = -EOPNOTSUPP; > > > goto exit; > > > } > > > > > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > res = rtw_set_key(padapter, psecuritypriv, keyid, 1); > > > > > > if (res == _FAIL) > > > - ret = false; > > > + ret = -ENOMEM; > > > exit: > > > > > > return ret; > > > > No, this isn't right. This now returns 1 on success and negative > > error codes on error. > > > > There are a couple anti-patterns here: > > > > 1) Do nothing gotos > > 2) Mixing error paths and success paths. > > > > If you avoid mixing error paths and success paths then you get a pattern > > called: "Solid return zero." This is where the end of the function has > > a very chunky "return 0;" to mark that it is successful. You want that. > > Some people do a "if (ret == 0) return ret;". Nope. "return ret;" is > > not chunky. > > > > regards, > > dan carpenter > > > > Hi Dan, > > Thank you for the review firstly, much appreciated. > > I'm happy of course to rewrite this to address any concerns, but > I was hoping I could clarify what you've said though? Apologies if I've > missed it, but how is this function now returning 1 on success? It sets > ret to 0 (success) at the start and then sets it to one of two negative > error codes depending on what happens. Am I missing something here? > (Perfectly possible that I am). > > In terms of do nothing gotos, do you mean gotos that just set an error > code then jump to the end? If you'd prefer, as the function just returns > right after the exit label, I can just return the codes directly and have > a 'return 0;' like you say above? > > Thanks as always for your insight. Yes, you've got it right. I think Dan is suggesting something like the below, but not necessarily in a single patch: --- drivers/staging/r8188eu/core/rtw_ioctl_set.c | 38 ++++++++++++---------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c index 17f6bcbeebf42..2736bbce83b5b 100644 --- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c +++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c @@ -390,44 +390,38 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11 return ret; } -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) +int rtw_set_802_11_add_wep(struct adapter *padapter, + struct ndis_802_11_wep *wep) { - int keyid, res; - struct security_priv *psecuritypriv = &padapter->securitypriv; - u8 ret = _SUCCESS; + int keyid; + struct security_priv *secpriv = &padapter->securitypriv; keyid = wep->KeyIndex & 0x3fffffff; - - if (keyid >= 4) { - ret = false; - goto exit; - } + if (keyid >= 4) + return -EOPNOTSUPP; switch (wep->KeyLength) { case 5: - psecuritypriv->dot11PrivacyAlgrthm = _WEP40_; + secpriv->dot11PrivacyAlgrthm = _WEP40_; break; case 13: - psecuritypriv->dot11PrivacyAlgrthm = _WEP104_; + secpriv->dot11PrivacyAlgrthm = _WEP104_; break; default: - psecuritypriv->dot11PrivacyAlgrthm = _NO_PRIVACY_; + secpriv->dot11PrivacyAlgrthm = _NO_PRIVACY_; break; } - memcpy(&psecuritypriv->dot11DefKey[keyid].skey[0], &wep->KeyMaterial, wep->KeyLength); + memcpy(secpriv->dot11DefKey[keyid].skey, &wep->KeyMaterial, + wep->KeyLength); - psecuritypriv->dot11DefKeylen[keyid] = wep->KeyLength; + secpriv->dot11DefKeylen[keyid] = wep->KeyLength; + secpriv->dot11PrivacyKeyIndex = keyid; - psecuritypriv->dot11PrivacyKeyIndex = keyid; + if (rtw_set_key(padapter, secpriv, keyid, 1) == _FAIL) + return -ENOMEM; - res = rtw_set_key(padapter, psecuritypriv, keyid, 1); - - if (res == _FAIL) - ret = false; -exit: - - return ret; + return 0; } /* ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-31 17:12 ` Joe Perches @ 2022-08-01 9:17 ` Dan Carpenter 2022-08-02 23:26 ` Phillip Potter 1 sibling, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2022-08-01 9:17 UTC (permalink / raw) To: Joe Perches Cc: Phillip Potter, gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Sun, Jul 31, 2022 at 10:12:56AM -0700, Joe Perches wrote: > > I'm happy of course to rewrite this to address any concerns, but > > I was hoping I could clarify what you've said though? Apologies if I've > > missed it, but how is this function now returning 1 on success? It sets > > ret to 0 (success) at the start and then sets it to one of two negative > > error codes depending on what happens. Am I missing something here? > > (Perfectly possible that I am). > > > > In terms of do nothing gotos, do you mean gotos that just set an error > > code then jump to the end? If you'd prefer, as the function just returns > > right after the exit label, I can just return the codes directly and have > > a 'return 0;' like you say above? > > > > Thanks as always for your insight. > > Yes, you've got it right. > > I think Dan is suggesting something like the below, but > not necessarily in a single patch: I always like your style, Joe. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-31 17:12 ` Joe Perches 2022-08-01 9:17 ` Dan Carpenter @ 2022-08-02 23:26 ` Phillip Potter 1 sibling, 0 replies; 12+ messages in thread From: Phillip Potter @ 2022-08-02 23:26 UTC (permalink / raw) To: Joe Perches Cc: Dan Carpenter, gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Sun, Jul 31, 2022 at 10:12:56AM -0700, Joe Perches wrote: > Yes, you've got it right. > > I think Dan is suggesting something like the below, but > not necessarily in a single patch: > --- > drivers/staging/r8188eu/core/rtw_ioctl_set.c | 38 ++++++++++++---------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c > index 17f6bcbeebf42..2736bbce83b5b 100644 > --- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c > +++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c > @@ -390,44 +390,38 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11 > return ret; > } > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > +int rtw_set_802_11_add_wep(struct adapter *padapter, > + struct ndis_802_11_wep *wep) > { > - int keyid, res; > - struct security_priv *psecuritypriv = &padapter->securitypriv; > - u8 ret = _SUCCESS; > + int keyid; > + struct security_priv *secpriv = &padapter->securitypriv; > > keyid = wep->KeyIndex & 0x3fffffff; > - > - if (keyid >= 4) { > - ret = false; > - goto exit; > - } > + if (keyid >= 4) > + return -EOPNOTSUPP; > > switch (wep->KeyLength) { > case 5: > - psecuritypriv->dot11PrivacyAlgrthm = _WEP40_; > + secpriv->dot11PrivacyAlgrthm = _WEP40_; > break; > case 13: > - psecuritypriv->dot11PrivacyAlgrthm = _WEP104_; > + secpriv->dot11PrivacyAlgrthm = _WEP104_; > break; > default: > - psecuritypriv->dot11PrivacyAlgrthm = _NO_PRIVACY_; > + secpriv->dot11PrivacyAlgrthm = _NO_PRIVACY_; > break; > } > > - memcpy(&psecuritypriv->dot11DefKey[keyid].skey[0], &wep->KeyMaterial, wep->KeyLength); > + memcpy(secpriv->dot11DefKey[keyid].skey, &wep->KeyMaterial, > + wep->KeyLength); > > - psecuritypriv->dot11DefKeylen[keyid] = wep->KeyLength; > + secpriv->dot11DefKeylen[keyid] = wep->KeyLength; > + secpriv->dot11PrivacyKeyIndex = keyid; > > - psecuritypriv->dot11PrivacyKeyIndex = keyid; > + if (rtw_set_key(padapter, secpriv, keyid, 1) == _FAIL) > + return -ENOMEM; > > - res = rtw_set_key(padapter, psecuritypriv, keyid, 1); > - > - if (res == _FAIL) > - ret = false; > -exit: > - > - return ret; > + return 0; > } > > /* > Hi Joe, Thanks for the suggestion, this is pretty much what I'd interpreted from Dan's advice in the meantime. I will prepare a V2 tomorrow. Regards, Phil ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-30 18:36 ` Phillip Potter 2022-07-31 17:12 ` Joe Perches @ 2022-08-01 7:00 ` Dan Carpenter 1 sibling, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2022-08-01 7:00 UTC (permalink / raw) To: Phillip Potter Cc: gregkh, Larry.Finger, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Sat, Jul 30, 2022 at 07:36:57PM +0100, Phillip Potter wrote: > On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote: > > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote: > > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > { > > > int keyid, res; > > > struct security_priv *psecuritypriv = &padapter->securitypriv; > > > - u8 ret = _SUCCESS; > > > + int ret = 0; > > > > > > keyid = wep->KeyIndex & 0x3fffffff; > > > > > > if (keyid >= 4) { > > > - ret = false; > > > + ret = -EOPNOTSUPP; > > > goto exit; > > > } > > > > > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep) > > > res = rtw_set_key(padapter, psecuritypriv, keyid, 1); > > > > > > if (res == _FAIL) > > > - ret = false; > > > + ret = -ENOMEM; > > > exit: > > > > > > return ret; > > > > No, this isn't right. This now returns 1 on success and negative > > error codes on error. > > > > There are a couple anti-patterns here: > > > > 1) Do nothing gotos > > 2) Mixing error paths and success paths. > > > > If you avoid mixing error paths and success paths then you get a pattern > > called: "Solid return zero." This is where the end of the function has > > a very chunky "return 0;" to mark that it is successful. You want that. > > Some people do a "if (ret == 0) return ret;". Nope. "return ret;" is > > not chunky. > > > > regards, > > dan carpenter > > > > Hi Dan, > > Thank you for the review firstly, much appreciated. > > I'm happy of course to rewrite this to address any concerns, but > I was hoping I could clarify what you've said though? Apologies if I've > missed it, but how is this function now returning 1 on success? It sets > ret to 0 (success) at the start and then sets it to one of two negative > error codes depending on what happens. Am I missing something here? > (Perfectly possible that I am). You're right. I misread "res" as "ret". It's another anti-pattern to have two "ret" variables. The code is fine but so ugly for no reason. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-07-28 23:11 ` [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics Phillip Potter 2022-07-29 6:48 ` Dan Carpenter @ 2022-08-02 20:10 ` Pavel Skripkin 2022-08-02 22:15 ` Phillip Potter 1 sibling, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-08-02 20:10 UTC (permalink / raw) To: Phillip Potter, gregkh Cc: Larry.Finger, dan.carpenter, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel Hi Phillip, Phillip Potter <phil@philpotter.co.uk> says: > - if (!rtw_set_802_11_add_wep(padapter, &wep)) { > + if (rtw_set_802_11_add_wep(padapter, &wep)) { > if (rf_on == pwrpriv->rf_pwrstate) > ret = -EOPNOTSUPP; > goto exit; is it intentional to ignore an error in case of rf_on != pwrpriv->rf_pwrstate? With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics 2022-08-02 20:10 ` Pavel Skripkin @ 2022-08-02 22:15 ` Phillip Potter 0 siblings, 0 replies; 12+ messages in thread From: Phillip Potter @ 2022-08-02 22:15 UTC (permalink / raw) To: Pavel Skripkin Cc: gregkh, Larry.Finger, dan.carpenter, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On Tue, Aug 02, 2022 at 11:10:21PM +0300, Pavel Skripkin wrote: > Hi Phillip, > > Phillip Potter <phil@philpotter.co.uk> says: > > - if (!rtw_set_802_11_add_wep(padapter, &wep)) { > > + if (rtw_set_802_11_add_wep(padapter, &wep)) { > > if (rf_on == pwrpriv->rf_pwrstate) > > ret = -EOPNOTSUPP; > > goto exit; > > is it intentional to ignore an error in case of rf_on != > pwrpriv->rf_pwrstate? > > Hi Pavel, Somewhat yes, in the sense that this is existing behaviour and changing it is a semantic change in the driver, thus arguably outside the scope of a patch/patch set that is intended to just focus on error code handling (moving from _SUCCESS/_FAIL to 0 and -EWHATEVER). Not fixed to that by any means though, if you would prefer it be restructured as well. I need to do a V2 for this anyway. Regards, Phil ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] staging: r8188eu: more error code cleanups 2022-07-28 23:11 [PATCH 0/2] staging: r8188eu: more error code cleanups Phillip Potter 2022-07-28 23:11 ` [PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup Phillip Potter 2022-07-28 23:11 ` [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics Phillip Potter @ 2022-07-29 6:09 ` Philipp Hortmann 2 siblings, 0 replies; 12+ messages in thread From: Philipp Hortmann @ 2022-07-29 6:09 UTC (permalink / raw) To: Phillip Potter, gregkh Cc: Larry.Finger, dan.carpenter, paskripkin, martin, straube.linux, fmdefrancesco, abdun.nihaal, linux-staging, linux-kernel On 7/29/22 01:11, Phillip Potter wrote: > This small series addresses a cleanup suggestion discussed by Greg > Kroah-Hartman and Dan Carpenter, and also includes another function > conversion. It would be a larger series, but my time being what it is, > I intend to chip away at these slowly and steadily. > > Phillip Potter (2): > staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup > staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics > > drivers/staging/r8188eu/core/rtw_ioctl_set.c | 8 ++++---- > drivers/staging/r8188eu/core/rtw_pwrctrl.c | 2 +- > drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +- > drivers/staging/r8188eu/os_dep/ioctl_linux.c | 5 ++--- > 4 files changed, 8 insertions(+), 9 deletions(-) > Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-08-02 23:26 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-28 23:11 [PATCH 0/2] staging: r8188eu: more error code cleanups Phillip Potter 2022-07-28 23:11 ` [PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup Phillip Potter 2022-07-28 23:11 ` [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics Phillip Potter 2022-07-29 6:48 ` Dan Carpenter 2022-07-30 18:36 ` Phillip Potter 2022-07-31 17:12 ` Joe Perches 2022-08-01 9:17 ` Dan Carpenter 2022-08-02 23:26 ` Phillip Potter 2022-08-01 7:00 ` Dan Carpenter 2022-08-02 20:10 ` Pavel Skripkin 2022-08-02 22:15 ` Phillip Potter 2022-07-29 6:09 ` [PATCH 0/2] staging: r8188eu: more error code cleanups Philipp Hortmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.