diff mbox series

staging: r8188eu: convert three functions to bool

Message ID 20221105093916.8255-1-straube.linux@gmail.com
State New
Headers
Series staging: r8188eu: convert three functions to bool |

Commit Message

Michael Straube Nov. 5, 2022, 9:39 a.m. UTC 
  The functions

is_client_associated_to_ap()
is_client_associated_to_ibss()
is_IBSS_empty()

return boolean values. Convert their return type to bool and replace
_FAIL, which is defined as 0, with false. Another step to get rid of
_SUCCESS / _FAIL.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_wlan_util.c   | 18 +++++++++---------
 drivers/staging/r8188eu/include/rtw_mlme_ext.h |  6 +++---
 2 files changed, 12 insertions(+), 12 deletions(-)

Comments

Phillip Potter Nov. 5, 2022, 11:06 a.m. UTC | #1 
On Sat, Nov 05, 2022 at 10:39:16AM +0100, Michael Straube wrote:
> The functions
> 
> is_client_associated_to_ap()
> is_client_associated_to_ibss()
> is_IBSS_empty()
> 
> return boolean values. Convert their return type to bool and replace
> _FAIL, which is defined as 0, with false. Another step to get rid of
> _SUCCESS / _FAIL.
> 
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_wlan_util.c   | 18 +++++++++---------
>  drivers/staging/r8188eu/include/rtw_mlme_ext.h |  6 +++---
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> index e50631848cab..c95438a12b59 100644
> --- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> +++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> @@ -331,35 +331,35 @@ u16 get_beacon_interval(struct wlan_bssid_ex *bss)
>  	return le16_to_cpu(val);
>  }
>  
> -int is_client_associated_to_ap(struct adapter *padapter)
> +bool is_client_associated_to_ap(struct adapter *padapter)
>  {
>  	struct mlme_ext_priv	*pmlmeext;
>  	struct mlme_ext_info	*pmlmeinfo;
>  
>  	if (!padapter)
> -		return _FAIL;
> +		return false;
>  
>  	pmlmeext = &padapter->mlmeextpriv;
>  	pmlmeinfo = &pmlmeext->mlmext_info;
>  
>  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
>  		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;
>  }
>  
> -int is_client_associated_to_ibss(struct adapter *padapter)
> +bool is_client_associated_to_ibss(struct adapter *padapter)
>  {
>  	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
>  	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
>  
>  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
>  		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;
>  }
>  
> -int is_IBSS_empty(struct adapter *padapter)
> +bool is_IBSS_empty(struct adapter *padapter)
>  {
>  	unsigned int i;
>  	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
> @@ -367,7 +367,7 @@ int is_IBSS_empty(struct adapter *padapter)
>  
>  	for (i = IBSS_START_MAC_ID; i < NUM_STA; i++) {
>  		if (pmlmeinfo->FW_sta_info[i].status == 1)
> -			return _FAIL;
> +			return false;
>  	}
>  	return true;
>  }
> diff --git a/drivers/staging/r8188eu/include/rtw_mlme_ext.h b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> index e234a3b9af6f..7652e72a03f4 100644
> --- a/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> +++ b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> @@ -432,9 +432,9 @@ void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
>  u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork);
>  u16 get_beacon_interval(struct wlan_bssid_ex *bss);
>  
> -int is_client_associated_to_ap(struct adapter *padapter);
> -int is_client_associated_to_ibss(struct adapter *padapter);
> -int is_IBSS_empty(struct adapter *padapter);
> +bool is_client_associated_to_ap(struct adapter *padapter);
> +bool is_client_associated_to_ibss(struct adapter *padapter);
> +bool is_IBSS_empty(struct adapter *padapter);
>  
>  unsigned char check_assoc_AP(u8 *pframe, uint len);
>  
> -- 
> 2.38.0
> 

Hi Michael,

Thanks for the patch. Just my personal opinion, but I would prefer to
keep the return type as int, and have 0 for success and then _FAIL
replaced with an appropriate error code depending on the circumstance,
(e.g. -ENOMEM). This is generally the convention elsewhere in the
kernel. Others may not agree though.

All the best,
Phil
Greg KH Nov. 5, 2022, 11:25 a.m. UTC | #2 
On Sat, Nov 05, 2022 at 11:06:56AM +0000, Phillip Potter wrote:
> On Sat, Nov 05, 2022 at 10:39:16AM +0100, Michael Straube wrote:
> > The functions
> > 
> > is_client_associated_to_ap()
> > is_client_associated_to_ibss()
> > is_IBSS_empty()
> > 
> > return boolean values. Convert their return type to bool and replace
> > _FAIL, which is defined as 0, with false. Another step to get rid of
> > _SUCCESS / _FAIL.
> > 
> > Signed-off-by: Michael Straube <straube.linux@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_wlan_util.c   | 18 +++++++++---------
> >  drivers/staging/r8188eu/include/rtw_mlme_ext.h |  6 +++---
> >  2 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> > index e50631848cab..c95438a12b59 100644
> > --- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> > +++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> > @@ -331,35 +331,35 @@ u16 get_beacon_interval(struct wlan_bssid_ex *bss)
> >  	return le16_to_cpu(val);
> >  }
> >  
> > -int is_client_associated_to_ap(struct adapter *padapter)
> > +bool is_client_associated_to_ap(struct adapter *padapter)
> >  {
> >  	struct mlme_ext_priv	*pmlmeext;
> >  	struct mlme_ext_info	*pmlmeinfo;
> >  
> >  	if (!padapter)
> > -		return _FAIL;
> > +		return false;
> >  
> >  	pmlmeext = &padapter->mlmeextpriv;
> >  	pmlmeinfo = &pmlmeext->mlmext_info;
> >  
> >  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
> >  		return true;
> > -	else
> > -		return _FAIL;
> > +
> > +	return false;
> >  }
> >  
> > -int is_client_associated_to_ibss(struct adapter *padapter)
> > +bool is_client_associated_to_ibss(struct adapter *padapter)
> >  {
> >  	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
> >  	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
> >  
> >  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
> >  		return true;
> > -	else
> > -		return _FAIL;
> > +
> > +	return false;
> >  }
> >  
> > -int is_IBSS_empty(struct adapter *padapter)
> > +bool is_IBSS_empty(struct adapter *padapter)
> >  {
> >  	unsigned int i;
> >  	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
> > @@ -367,7 +367,7 @@ int is_IBSS_empty(struct adapter *padapter)
> >  
> >  	for (i = IBSS_START_MAC_ID; i < NUM_STA; i++) {
> >  		if (pmlmeinfo->FW_sta_info[i].status == 1)
> > -			return _FAIL;
> > +			return false;
> >  	}
> >  	return true;
> >  }
> > diff --git a/drivers/staging/r8188eu/include/rtw_mlme_ext.h b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> > index e234a3b9af6f..7652e72a03f4 100644
> > --- a/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> > +++ b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> > @@ -432,9 +432,9 @@ void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
> >  u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork);
> >  u16 get_beacon_interval(struct wlan_bssid_ex *bss);
> >  
> > -int is_client_associated_to_ap(struct adapter *padapter);
> > -int is_client_associated_to_ibss(struct adapter *padapter);
> > -int is_IBSS_empty(struct adapter *padapter);
> > +bool is_client_associated_to_ap(struct adapter *padapter);
> > +bool is_client_associated_to_ibss(struct adapter *padapter);
> > +bool is_IBSS_empty(struct adapter *padapter);
> >  
> >  unsigned char check_assoc_AP(u8 *pframe, uint len);
> >  
> > -- 
> > 2.38.0
> > 
> 
> Hi Michael,
> 
> Thanks for the patch. Just my personal opinion, but I would prefer to
> keep the return type as int, and have 0 for success and then _FAIL
> replaced with an appropriate error code depending on the circumstance,
> (e.g. -ENOMEM). This is generally the convention elsewhere in the
> kernel. Others may not agree though.

For functions that are just returning true/false like this (and the
function name is something like "is_foo()"), a boolean type is fine and
makes more sense as you will only check it in an "if ()" statement block
like how these are being used.

However, these function names are horrible for being in the global
namespace, but that's a totally different issue that can be fixed up in
a later patch.

thanks,

greg k-h
Philipp Hortmann Nov. 6, 2022, 8:36 a.m. UTC | #3 
On 11/5/22 10:39, Michael Straube wrote:
> The functions
> 
> is_client_associated_to_ap()
> is_client_associated_to_ibss()
> is_IBSS_empty()
> 
> return boolean values. Convert their return type to bool and replace
> _FAIL, which is defined as 0, with false. Another step to get rid of
> _SUCCESS / _FAIL.
> 
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_wlan_util.c   | 18 +++++++++---------
>   drivers/staging/r8188eu/include/rtw_mlme_ext.h |  6 +++---
>   2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> index e50631848cab..c95438a12b59 100644
> --- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> +++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> @@ -331,35 +331,35 @@ u16 get_beacon_interval(struct wlan_bssid_ex *bss)
>   	return le16_to_cpu(val);
>   }
>   
> -int is_client_associated_to_ap(struct adapter *padapter)
> +bool is_client_associated_to_ap(struct adapter *padapter)
>   {
>   	struct mlme_ext_priv	*pmlmeext;
>   	struct mlme_ext_info	*pmlmeinfo;
>   
>   	if (!padapter)
> -		return _FAIL;
> +		return false;
>   
>   	pmlmeext = &padapter->mlmeextpriv;
>   	pmlmeinfo = &pmlmeext->mlmext_info;
>   
>   	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
>   		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;
>   }
>   
> -int is_client_associated_to_ibss(struct adapter *padapter)
> +bool is_client_associated_to_ibss(struct adapter *padapter)
>   {
>   	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
>   	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
>   
>   	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
>   		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;
>   }
>   
> -int is_IBSS_empty(struct adapter *padapter)
> +bool is_IBSS_empty(struct adapter *padapter)
>   {
>   	unsigned int i;
>   	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
> @@ -367,7 +367,7 @@ int is_IBSS_empty(struct adapter *padapter)
>   
>   	for (i = IBSS_START_MAC_ID; i < NUM_STA; i++) {
>   		if (pmlmeinfo->FW_sta_info[i].status == 1)
> -			return _FAIL;
> +			return false;
>   	}
>   	return true;
>   }
> diff --git a/drivers/staging/r8188eu/include/rtw_mlme_ext.h b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> index e234a3b9af6f..7652e72a03f4 100644
> --- a/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> +++ b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
> @@ -432,9 +432,9 @@ void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
>   u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork);
>   u16 get_beacon_interval(struct wlan_bssid_ex *bss);
>   
> -int is_client_associated_to_ap(struct adapter *padapter);
> -int is_client_associated_to_ibss(struct adapter *padapter);
> -int is_IBSS_empty(struct adapter *padapter);
> +bool is_client_associated_to_ap(struct adapter *padapter);
> +bool is_client_associated_to_ibss(struct adapter *padapter);
> +bool is_IBSS_empty(struct adapter *padapter);
>   
>   unsigned char check_assoc_AP(u8 *pframe, uint len);
>   
Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150
Joe Perches Nov. 6, 2022, 8:58 a.m. UTC | #4 
On Sat, 2022-11-05 at 10:39 +0100, Michael Straube wrote:
> The functions
> 
> is_client_associated_to_ap()
> is_client_associated_to_ibss()
> is_IBSS_empty()
> 
> return boolean values. Convert their return type to bool and replace
> _FAIL, which is defined as 0, with false. Another step to get rid of
> _SUCCESS / _FAIL.

yay.

> diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
[]
> +bool is_client_associated_to_ap(struct adapter *padapter)
>  {
>  	struct mlme_ext_priv	*pmlmeext;
>  	struct mlme_ext_info	*pmlmeinfo;
>  
>  	if (!padapter)
> -		return _FAIL;
> +		return false;
>  
>  	pmlmeext = &padapter->mlmeextpriv;
>  	pmlmeinfo = &pmlmeext->mlmext_info;
>  
>  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
>  		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;

instead of

	if (foo)
		return true;
	return false;

These could be:

	return (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) &&
	       ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE);

> +bool is_client_associated_to_ibss(struct adapter *padapter)
>  {
>  	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
>  	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
>  
>  	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
>  		return true;
> -	else
> -		return _FAIL;
> +
> +	return false;

and

	return (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) &&
	       ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE);
Michael Straube Nov. 6, 2022, 9:37 a.m. UTC | #5 
On 11/5/22 12:25, Greg KH wrote:
> However, these function names are horrible for being in the global
> namespace, but that's a totally different issue that can be fixed up in
> a later patch.
> 

I'll send a patch to address that after this patch is accepted.

Thanks,
Michael
Michael Straube Nov. 6, 2022, 9:39 a.m. UTC | #6 
On 11/6/22 09:58, Joe Perches wrote:
> On Sat, 2022-11-05 at 10:39 +0100, Michael Straube wrote:
>> The functions
>>
>> is_client_associated_to_ap()
>> is_client_associated_to_ibss()
>> is_IBSS_empty()
>>
>> return boolean values. Convert their return type to bool and replace
>> _FAIL, which is defined as 0, with false. Another step to get rid of
>> _SUCCESS / _FAIL.
> 
> yay.
> 
>> diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> []
>> +bool is_client_associated_to_ap(struct adapter *padapter)
>>   {
>>   	struct mlme_ext_priv	*pmlmeext;
>>   	struct mlme_ext_info	*pmlmeinfo;
>>   
>>   	if (!padapter)
>> -		return _FAIL;
>> +		return false;
>>   
>>   	pmlmeext = &padapter->mlmeextpriv;
>>   	pmlmeinfo = &pmlmeext->mlmext_info;
>>   
>>   	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
>>   		return true;
>> -	else
>> -		return _FAIL;
>> +
>> +	return false;
> 
> instead of
> 
> 	if (foo)
> 		return true;
> 	return false;
> 
> These could be:
> 
> 	return (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) &&
> 	       ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE);
> 
>> +bool is_client_associated_to_ibss(struct adapter *padapter)
>>   {
>>   	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
>>   	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
>>   
>>   	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
>>   		return true;
>> -	else
>> -		return _FAIL;
>> +
>> +	return false;
> 
> and
> 
> 	return (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) &&
> 	       ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE);
> 
> 

I'll keep this in mind for a future patch.

Thanks,
Michael
diff mbox series

Patch

diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
index e50631848cab..c95438a12b59 100644
--- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
+++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
@@ -331,35 +331,35 @@  u16 get_beacon_interval(struct wlan_bssid_ex *bss)
 	return le16_to_cpu(val);
 }
 
-int is_client_associated_to_ap(struct adapter *padapter)
+bool is_client_associated_to_ap(struct adapter *padapter)
 {
 	struct mlme_ext_priv	*pmlmeext;
 	struct mlme_ext_info	*pmlmeinfo;
 
 	if (!padapter)
-		return _FAIL;
+		return false;
 
 	pmlmeext = &padapter->mlmeextpriv;
 	pmlmeinfo = &pmlmeext->mlmext_info;
 
 	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE))
 		return true;
-	else
-		return _FAIL;
+
+	return false;
 }
 
-int is_client_associated_to_ibss(struct adapter *padapter)
+bool is_client_associated_to_ibss(struct adapter *padapter)
 {
 	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
 
 	if ((pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS) && ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE))
 		return true;
-	else
-		return _FAIL;
+
+	return false;
 }
 
-int is_IBSS_empty(struct adapter *padapter)
+bool is_IBSS_empty(struct adapter *padapter)
 {
 	unsigned int i;
 	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
@@ -367,7 +367,7 @@  int is_IBSS_empty(struct adapter *padapter)
 
 	for (i = IBSS_START_MAC_ID; i < NUM_STA; i++) {
 		if (pmlmeinfo->FW_sta_info[i].status == 1)
-			return _FAIL;
+			return false;
 	}
 	return true;
 }
diff --git a/drivers/staging/r8188eu/include/rtw_mlme_ext.h b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
index e234a3b9af6f..7652e72a03f4 100644
--- a/drivers/staging/r8188eu/include/rtw_mlme_ext.h
+++ b/drivers/staging/r8188eu/include/rtw_mlme_ext.h
@@ -432,9 +432,9 @@  void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
 u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork);
 u16 get_beacon_interval(struct wlan_bssid_ex *bss);
 
-int is_client_associated_to_ap(struct adapter *padapter);
-int is_client_associated_to_ibss(struct adapter *padapter);
-int is_IBSS_empty(struct adapter *padapter);
+bool is_client_associated_to_ap(struct adapter *padapter);
+bool is_client_associated_to_ibss(struct adapter *padapter);
+bool is_IBSS_empty(struct adapter *padapter);
 
 unsigned char check_assoc_AP(u8 *pframe, uint len);