Re: [RFC][PATCH v1 1/1] Add WARN_ONCE_RETURN macro

[Casey Schaufler]

On 8/14/2015 6:43 AM, Andy Shevchenko wrote:
> The rationale is to replace the
> 	if (condition) {
> 		WARN_ONCE(1, ...);
> 		return value;
> 	}
> by
> 	WARN_ONCE_RETURN(condition, value, ...);

The kernel coding style says:

Things to avoid when using macros:

1) macros that affect control flow:

	#define FOO(x)					\
		do {					\
			if (blah(x) < 0)		\
				return -EBUGGERED;	\
		} while(0)

is a _very_ bad idea.  It looks like a function call but exits the "calling"
function; don't break the internal parsers of those who will read the code.

So no, I won't be taking this unless there's a change in
the coding guidelines, which I coincidentally agree with.

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  include/asm-generic/bug.h  | 17 +++++++++++++++++
>  security/smack/smack_lsm.c |  5 +----
>  2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 630dd23..fbf20fd 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -126,6 +126,18 @@ extern void warn_slowpath_null(const char *file, const int line);
>  	unlikely(__ret_warn_once);				\
>  })
>  
> +#define WARN_ONCE_RETURN(condition, value, format...)		\
> +do {								\
> +	static bool __section(.data.unlikely) __warned;		\
> +	int __ret_warn_once = !!(condition);			\
> +								\
> +	if (unlikely(__ret_warn_once)) {			\
> +		if (WARN(!__warned, format)) 			\
> +			__warned = true;			\
> +		return value;					\
> +	}							\
> +} while (0)
> +
>  #define WARN_TAINT_ONCE(condition, taint, format...)	({	\
>  	static bool __section(.data.unlikely) __warned;		\
>  	int __ret_warn_once = !!(condition);			\
> @@ -162,6 +174,11 @@ extern void warn_slowpath_null(const char *file, const int line);
>  
>  #define WARN_ON_ONCE(condition) WARN_ON(condition)
>  #define WARN_ONCE(condition, format...) WARN(condition, format)
> +#define WARN_ONCE_RETURN(condition, value, format...)			\
> +do {									\
> +	if (WARN(condition, format))					\
> +		return value;						\
> +} while (0)
>  #define WARN_TAINT(condition, taint, format...) WARN(condition, format)
>  #define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 54fb3a1..4160d0e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4401,10 +4401,7 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
>  	struct smack_known *skp;
>  	char *rule = vrule;
>  
> -	if (unlikely(!rule)) {
> -		WARN_ONCE(1, "Smack: missing rule\n");
> -		return -ENOENT;
> -	}
> +	WARN_ONCE_RETURN(unlikely(!rule), -ENOENT, "Smack: missing rule\n");
>  
>  	if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER)
>  		return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/