linux-kernel - Re: [PATCH] staging: greybus: eclose macro in a do

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:   Sat, 11 Mar 2023 13:10:59 -0600
From:   Alex Elder <elder@...e.org>
To:     Menna Mahmoud <eng.mennamahmoud.mm@...il.com>,
        outreachy@...ts.linux.dev
Cc:     pure.logic@...us-software.ie, johan@...nel.org, elder@...nel.org,
        gregkh@...uxfoundation.org, greybus-dev@...ts.linaro.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: greybus: eclose macro in a do - while loop

On 3/11/23 7:59 AM, Menna Mahmoud wrote:
> " ERROR: Macros with multiple statements should be enclosed in a do -
> while loop"
> 
> Reported by checkpath.

This is also not an issue that should be "fixed."

If you look at where that macro is expanded, you see
that its purpose is simply to reduce the possibility
of some errors by enclosing some much-duplicated code
in this macro.  The expansion is at the top level of
the source file, so a "do...while" loop ends up being
an error.

When looking at the output of checkpatch, assume it's
giving you clues about problems that one *might* like to
fix.  Its suggestions are most often reasonable, but in
some cases (like this one) it's just not smart enough
to recognize the problem that comes from following its
advice.

Make sure you understand exactly what happens when
you make a change.  That means understanding the
code, and then it means ensuring that the fix passes
at least a compile test, and if possible an actual
execution test.

					-Alex

> 
> do loop with the conditional expression set to a constant
> value of zero (0).This creates a loop that
> will execute exactly one time.This is a coding idiom that
> allows a multi-line macro to be used anywhere
> that a single statement can be used.
> 
> So, enclose `gb_loopback_stats_attrs` macro in do - while (0) to
> fix checkpath error
> 
> Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@...il.com>
> ---
>   drivers/staging/greybus/loopback.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 1a61fce98056..e86d50638cb5 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -162,10 +162,12 @@ static ssize_t name##_avg_show(struct device *dev,		\
>   }									\
>   static DEVICE_ATTR_RO(name##_avg)
>   
> -#define gb_loopback_stats_attrs(field)				\
> -	gb_loopback_ro_stats_attr(field, min, u);		\
> -	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> +#define gb_loopback_stats_attrs(field)					\
> +	do { \
> +		gb_loopback_ro_stats_attr(field, min, u);		\
> +		gb_loopback_ro_stats_attr(field, max, u);		\
> +		gb_loopback_ro_avg_attr(field);				\
> +	} while (0)
>   
>   #define gb_loopback_attr(field, type)					\
>   static ssize_t field##_show(struct device *dev,				\