xfs
[Top] [All Lists]

Re: [PATCH] xfs: add a few more verifier tests

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: add a few more verifier tests
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 20 Aug 2014 08:38:45 +1000
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53F3A07B.9040402@xxxxxxxxxxx>
References: <53F2C103.8030607@xxxxxxxxxx> <20140819181542.GA31177@xxxxxxxxxxxxx> <53F3A07B.9040402@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Aug 19, 2014 at 02:07:39PM -0500, Eric Sandeen wrote:
> On 8/19/14, 1:15 PM, Christoph Hellwig wrote:
> >> Anyway - bounds checking when we read from disk is a good thing!
> > 
> > Absolutelt!
> > 
> > Looks good modulo a few nitpicks below.
> > 
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > 
> >> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> >> index 4bffffe..a4a9e0e 100644
> >> --- a/fs/xfs/libxfs/xfs_alloc.c
> >> +++ b/fs/xfs/libxfs/xfs_alloc.c
> >> @@ -2209,6 +2209,10 @@ xfs_agf_verify(
> >>          be32_to_cpu(agf->agf_flcount) <= XFS_AGFL_SIZE(mp)))
> >>            return false;
> >>  
> >> +  if (!(be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) <= 
> >> XFS_BTREE_MAXLEVELS &&
> >> +        be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) <= 
> >> XFS_BTREE_MAXLEVELS))
> >> +          return false;
> > 
> > Maybe it's just me, but negated numeric comparisms always confuse the
> > hell out of me, why not simply:
> > 
> >     if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) > XFS_BTREE_MAXLEVELS)
> >             return false;
> >     if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) > XFS_BTREE_MAXLEVELS)
> >             return false;
> > 
> >> --- a/fs/xfs/libxfs/xfs_ialloc.c
> >> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> >> @@ -2051,6 +2051,8 @@ xfs_agi_verify(
> >>    if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum)))
> >>            return false;
> >>  
> >> +  if (!(be32_to_cpu(agi->agi_level) <= XFS_BTREE_MAXLEVELS))
> >> +          return false;
> > 
> > Same here.
> 
> yeah; just following the style of the functions as they exist today...
> 
>         if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) &&
>               XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
>               be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) 
> &&
> ...
> 
> dunno. Don't care too much either way, but consistency and all that...

I prefer the metho Christoph suggested - most of the verifiers use
that "single check per if statement" pattern because it makes the
checks being performed so much easier to read.

> Maybe the "AGF_GOOD_VERSION" required the negation, and it all got lumped
> together?

Those should probably be cleaned up - they were done like that
originally as a direct transcript from pre-existing code checks
to simplify review, not because it was "nice" code.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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