xfs
[Top] [All Lists]

Re: [PATCH 03/11] xfs: factor out xfs_dir2_leaf_find_stale

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 03/11] xfs: factor out xfs_dir2_leaf_find_stale
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 13 Jul 2011 16:49:36 +1000
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1310423573.7019.55.camel@doink>
References: <20110710204916.856267100@xxxxxxxxxxxxxxxxxxxxxx> <20110710205017.293539533@xxxxxxxxxxxxxxxxxxxxxx> <1310423573.7019.55.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon, Jul 11, 2011 at 05:32:53PM -0500, Alex Elder wrote:
> On Sun, 2011-07-10 at 16:49 -0400, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> OK, I guess this is fine.  I have a suggestion below
> but I see nothing truly wrong with what you have.
> 
> Reviewed-by: Alex Elder <aelder@xxxxxxx>
> 
> > Index: xfs/fs/xfs/xfs_dir2_leaf.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_dir2_leaf.c 2011-07-09 12:07:49.518499801 +0200
> > +++ xfs/fs/xfs/xfs_dir2_leaf.c      2011-07-09 12:19:48.171796748 +0200
> > @@ -148,6 +148,36 @@ xfs_dir2_block_to_leaf(
> >     return 0;
> >  }
> >  
> > +STATIC void
> > +xfs_dir2_leaf_find_stale(
> > +   struct xfs_dir2_leaf    *leaf,
> > +   int                     index,
> > +   int                     *lowstale,
> > +   int                     *highstale)
> > +{
> > +   /*
> > +    * Find the first stale entry before our index, if any.
> > +    */
> > +   for (*lowstale = index - 1;
> > +        *lowstale >= 0 &&
> > +           leaf->ents[*lowstale].address !=
> > +           cpu_to_be32(XFS_DIR2_NULL_DATAPTR);
> > +        --*lowstale)
> > +           continue;

Only thing I was conerned about was the indenting on these loops.
Something like this:

        for (*lowstale = index - 1;
             *lowstale >= 0 &&
                        leaf->ents[*lowstale].address !=
                        cpu_to_be32(XFS_DIR2_NULL_DATAPTR);
             --*lowstale)
                continue;

means that at a glance it is easy to separate the loop control
statements from the body of the loop just by indentation.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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