linux-kernel - Re: [PATCH 6/6] wlan-ng: clean up reused macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Jan 2021 12:33:56 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Johannes Czekay <johannes.czekay@....de>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        jbwyatt4@...il.com, gustavo@...eddedor.com,
        linux-kernel@...cs.fau.de, linux-kernel@...r.kernel.org,
        rkovhaev@...il.com, nicolai.fischer@....de, hqjagain@...il.com
Subject: Re: [PATCH 6/6] wlan-ng: clean up reused macros

On Mon, Jan 18, 2021 at 02:09:56AM +0100, Johannes Czekay wrote:
> This patch cleans up two "macro argument reuse" warnings by checkpatch.
> This should also make the code much more readable.
> 
> Signed-off-by: Johannes Czekay <johannes.czekay@....de>
> Co-developed-by: Nicolai Fischer <nicolai.fischer@....de>
> Signed-off-by: Nicolai Fischer <nicolai.fischer@....de>
> ---
>  drivers/staging/wlan-ng/p80211metastruct.h | 18 +-------
>  drivers/staging/wlan-ng/prism2mgmt.c       | 48 ++++++----------------
>  2 files changed, 14 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211metastruct.h b/drivers/staging/wlan-ng/p80211metastruct.h
> index 4adc64580185..e963227f797c 100644
> --- a/drivers/staging/wlan-ng/p80211metastruct.h
> +++ b/drivers/staging/wlan-ng/p80211metastruct.h
> @@ -114,22 +114,8 @@ struct p80211msg_dot11req_scan_results {
>  	struct p80211item_uint32 cfpollreq;
>  	struct p80211item_uint32 privacy;
>  	struct p80211item_uint32 capinfo;
> -	struct p80211item_uint32 basicrate1;
> -	struct p80211item_uint32 basicrate2;
> -	struct p80211item_uint32 basicrate3;
> -	struct p80211item_uint32 basicrate4;
> -	struct p80211item_uint32 basicrate5;
> -	struct p80211item_uint32 basicrate6;
> -	struct p80211item_uint32 basicrate7;
> -	struct p80211item_uint32 basicrate8;
> -	struct p80211item_uint32 supprate1;
> -	struct p80211item_uint32 supprate2;
> -	struct p80211item_uint32 supprate3;
> -	struct p80211item_uint32 supprate4;
> -	struct p80211item_uint32 supprate5;
> -	struct p80211item_uint32 supprate6;
> -	struct p80211item_uint32 supprate7;
> -	struct p80211item_uint32 supprate8;
> +	struct p80211item_uint32 basicrate[8];
> +	struct p80211item_uint32 supprate[8];
>  } __packed;
>  
>  struct p80211msg_dot11req_start {
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c
> index 1bd36dc2b7ff..8540c3336907 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -388,6 +388,7 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp)
>  	struct hfa384x_hscan_result_sub *item = NULL;
>  
>  	int count;
> +	int i;
>  
>  	req = msgp;
>  
> @@ -437,42 +438,17 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp)
>  		if (item->supprates[count] == 0)
>  			break;
>  
> -#define REQBASICRATE(N) \
> -	do { \
> -		if ((count >= (N)) && DOT11_RATE5_ISBASIC_GET(	\
> -			item->supprates[(N) - 1])) { \
> -			req->basicrate ## N .data = item->supprates[(N) - 1]; \
> -			req->basicrate ## N .status = \
> -				P80211ENUM_msgitem_status_data_ok; \
> -		} \
> -	} while (0)
> -
> -	REQBASICRATE(1);
> -	REQBASICRATE(2);
> -	REQBASICRATE(3);
> -	REQBASICRATE(4);
> -	REQBASICRATE(5);
> -	REQBASICRATE(6);
> -	REQBASICRATE(7);
> -	REQBASICRATE(8);
> -
> -#define REQSUPPRATE(N) \
> -	do { \
> -		if (count >= (N)) {					\
> -			req->supprate ## N .data = item->supprates[(N) - 1]; \
> -			req->supprate ## N .status = \
> -				P80211ENUM_msgitem_status_data_ok; \
> -		} \
> -	} while (0)
> -
> -	REQSUPPRATE(1);
> -	REQSUPPRATE(2);
> -	REQSUPPRATE(3);
> -	REQSUPPRATE(4);
> -	REQSUPPRATE(5);
> -	REQSUPPRATE(6);
> -	REQSUPPRATE(7);
> -	REQSUPPRATE(8);
> +	for (i = 0; i < 8; i++) {
> +		if (count > 1) {

This should be "i" instead of "1".  And conditions are more readable if
you put the part that changes first "if (variable < limit)" to avoid
backwards think.

	if (i < count) {

Or you could reverse it the other way:

	if (i >= count)
		break;

> +			if (DOT11_RATE5_ISBASIC_GET(item->supprates[i])) {
> +				req->basicrate[i].data = item->supprates[i];
> +				req->basicrate[i].status = P80211ENUM_msgitem_status_data_ok;
> +			}
> +
> +			req->supprate[i].data = item->supprates[i];
> +			req->supprate[i].status = P80211ENUM_msgitem_status_data_ok;
> +		}
> +	}

It's sort of surprising that we can change how we write this information
but we don't have to change how it is read.  I guess presumably it's
just dumped as hex to debugfs or something like that...  Who knows.  :/

regards,
dan carpenter