Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption

[Paul E. McKenney]

On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote:
> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
> > +
> > +/*
> > + * Since detected data corruption should stop operation on the affected
> > + * structures, this returns false if the corruption condition is found.
> > + */
> > +#define CHECK_DATA_CORRUPTION(condition, fmt, ...)                      \
> > +       do {                                                             \
> > +               if (unlikely(condition)) {                               \
> > +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> > +                               pr_err(fmt, ##__VA_ARGS__);              \
> > +                               BUG();                                   \
> > +                       } else                                           \
> > +                               WARN(1, fmt, ##__VA_ARGS__);             \
> > +                       return false;                                    \
> > +               }                                                        \
> > +       } while (0)
> > +
> 
> I think the "return false" inside of the macro makes it easy to misread
> what is actually going on.
> 
> How about making it a macro that returns the condition argument?
> 
> #define CHECK_DATA_CORRUPTION(condition, fmt, ...)	\
> ({	\
> 	bool _condition = unlikely(condition);	\
> 	if (_condition) {	\
> 		...
> 	}	\
> 	_condition;	\
> })

That does look better, now that you mention it.  Kees, any objections?

							Thanx, Paul