xfs
[Top] [All Lists]

Re: [PATCH 19/25] xfs: add xfs_da_node verification

To: Phil White <pwhite@xxxxxxx>
Subject: Re: [PATCH 19/25] xfs: add xfs_da_node verification
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 31 Oct 2012 11:50:47 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121031002346.GB20337@xxxxxxxxxxxxxxxxxxxx>
References: <1351146854-19343-1-git-send-email-david@xxxxxxxxxxxxx> <1351146854-19343-20-git-send-email-david@xxxxxxxxxxxxx> <20121030133026.GE18595@xxxxxxxxxxxxxxxxxxxx> <20121030222332.GD29378@dastard> <20121031002346.GB20337@xxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Oct 30, 2012 at 05:23:47PM -0700, Phil White wrote:
> On Wed, Oct 31, 2012 at 09:23:32AM +1100, Dave Chinner wrote:
> > There's also another problem with this - endian swapping is missing.
> 
> Endian swapping doesn't matter.  include/linux/types.h defines __be16 as
> a __u16 and 0 is 0 is 0, no matter which order you put the bytes.

Except that the compiler based endian checks throw a error because
it is wrong ;)

(make C=2 CF="-D__CHECK_ENDIAN__" fs/xfs/xfs.ko)

> Doesn't hurt to make it clear though.
>  
> > xfs: add xfs_da_node verification
> > 
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_attr.c      |   22 ++++------
> >  fs/xfs/xfs_attr_leaf.c |   12 +++---
> >  fs/xfs/xfs_attr_leaf.h |    8 ++--
> >  fs/xfs/xfs_da_btree.c  |  109 
> > ++++++++++++++++++++++++++++++++++++------------
> >  fs/xfs/xfs_da_btree.h  |    3 ++
> >  fs/xfs/xfs_dir2_leaf.c |    2 +-
> >  fs/xfs/xfs_dir2_priv.h |    1 +
> >  7 files changed, 107 insertions(+), 50 deletions(-)
> 
> I'm a little surprised (and dismayed) that it passed xfstests with that.
> Presumably, it never ran into a case where level or count were > 0 on
> an invalid xfs_da_node.

Sure, xfstests doesn't usually trigger directory corruption, and so
it never would have had a chance to fail the verification
incorrectly. More than anything, what we are testing with xfstests
is that the verification doesn't introduce regressions, not that the
verification is 100% correct. Review is what catches the thinkos and
typos (as it has in this case), so I think the process is working as
it should.  ;)

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>