On Sun, Nov 10, 2019 at 05:17:58PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > Add a new macro, if_xfs_meta_bad, which we will use to integrate some > corruption reporting when the corruption test expression is true. This > will be used in the next patch to remove the ugly XFS_WANT_CORRUPT* > macros. > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > --- Ooh a new bikeshed... :) > fs/xfs/xfs_linux.h | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h > index 2271db4e8d66..d0fb1e612c42 100644 > --- a/fs/xfs/xfs_linux.h > +++ b/fs/xfs/xfs_linux.h > @@ -229,6 +229,10 @@ int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count, > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : assfail(NULL, #expr, __FILE__, __LINE__)) > > +#define xfs_meta_bad(mp, expr) \ > + (unlikely(expr) ? assfail((mp), #expr, __FILE__, __LINE__), \ > + true : false) > + > #else /* !DEBUG */ > > #ifdef XFS_WARN > @@ -236,13 +240,23 @@ int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count, > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : asswarn(NULL, #expr, __FILE__, __LINE__)) > > +#define xfs_meta_bad(mp, expr) \ > + (unlikely(expr) ? asswarn((mp), #expr, __FILE__, __LINE__), \ > + true : false) > + > #else /* !DEBUG && !XFS_WARN */ > > -#define ASSERT(expr) ((void)0) > +#define ASSERT(expr) ((void)0) > + > +#define xfs_meta_bad(mp, expr) \ > + (unlikely(expr) ? XFS_ERROR_REPORT(#expr, XFS_ERRLEVEL_LOW, (mp)), \ > + true : false) > > #endif /* XFS_WARN */ > #endif /* DEBUG */ > > +#define if_xfs_meta_bad(mp, expr) if (xfs_meta_bad((mp), (expr))) > + FWIW, 'xfs_meta_bad' doesn't really tell me anything about what the macro is for, particularly since the logic that determines whether metadata is bad is fed into it. IOW, I read that and expect the macro to actually do something generic to determine whether metadata is bad. Also having taken a quick look at the next patch, I agree with Christoph on embedding if logic into the macro itself, at least with respect to readability. It makes the code look like a typo/syntax error to me. :P I agree that the existing macros are ugly, but they at least express operational semantics reasonably well between [_RETURN|_GOTO]. If we really want to fix the latter bit, perhaps the best incremental step is to drop the branching logic and naming portion from the existing macros and leave everything else as is (from the commit logs, it sounds like this is more along the lines of your previous version, just without the name change). From there perhaps we can come up with better naming eventually. Just a thought. Brian > #define STATIC static noinline > > #ifdef CONFIG_XFS_RT >
