Re: [PATCH 0/2] xfs: pass mp to XFS_WANT_CORRUPTED_*

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 09, 2015 at 04:43:59PM -0500, Brian Foster wrote:
> On Tue, Feb 10, 2015 at 08:17:44AM +1100, Dave Chinner wrote:
> > On Mon, Feb 09, 2015 at 08:09:26AM -0500, Brian Foster wrote:
> > > On Mon, Feb 09, 2015 at 08:35:02AM +1100, Dave Chinner wrote:
> > > > On Fri, Feb 06, 2015 at 04:22:04PM -0600, Eric Sandeen wrote:
> > > > > These 2 patches provide information about which filesystem
> > > > > hit the error...
> > > > 
> > > > If we are going to touch every one of these macros, then can we
> > > > rename them to something a little shorter like XFS_CORRUPT_GOTO()
> > > > and XFS_CORRUPT_RETURN() at the same time? That will make the code a
> > > > little less eye-bleedy where there are lots of these statements,
> > > > and make formatting of complex checks a bit easier, too...
> > > > 
> > > 
> > > XFS_CORRUPT_DOSOMETHING() jumps out to me as indicate corruption if the
> > > logic statement evaluates as true rather than false. The latter (e.g.,
> > > assert-like logic) is how they work today, so that could be a bit
> > > confusing to somebody who isn't already familiar with how these macros
> > > work.
> > 
> > Someone not familiar with XFS conventions is already going to get
> > caught by "should be true" logic of these statements anyway as the
> > logic is the opposite of BUG_ON() and WARN_ON(). i.e.  BUG_ON(1)
> > will kill the kernel, while ASSERT(1) indicates everything is fine.
> > 
> 
> BUG_ON() and ASSERT() are self-explanatory, the latter being a pretty
> standard/common thing ('man assert'). As Eric mentioned, the WANT bit of
> the macro is what suggests assert-like semantics.
> 
> > I suggested shortening the macro because it makes the code that uses
> > it extensively shouty and hard to read because it splits logic
> > statements across lines regularly (e.g __xfs_dir3_data_check).  I
> > want to use this more extensively in verifiers to give better
> > corruption detection reporting, but the current macro will make the
> > verifier code rather ugly. Hence my suggestion to make it shorter,
> > neater and a little less shouty...
> > 
> 
> Sure, but ASSERT_CORRUPT_RET() is the same length as the example above.
> ASSERT_CORRUPT_GOTO() is only a few chars longer than the associated
> example. We could still use WANT over ASSERT I suppose to shorten it up
> further. Either of those are at least still self-explanatory in my
> opinion.

Thinking on it a bit further, the XFS_WANT_CORRUPTED macros have an
internal ASSERT in them, so they are effectively an ASSERT
statement. I could live with those names, especially as ASSERT is
something that can be compiled into production kernels via
CONFIG_XFS_WARN=y to turn them into error messages...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs