xfs
[Top] [All Lists]

Re: [PATCH 3/5] metadump: separate single block objects from multiblock

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/5] metadump: separate single block objects from multiblock objects
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 4 Feb 2014 09:19:49 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140203150907.GB6335@xxxxxxxxxxxxx>
References: <1390472635-17225-1-git-send-email-david@xxxxxxxxxxxxx> <1390472635-17225-4-git-send-email-david@xxxxxxxxxxxxx> <20140203150907.GB6335@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Feb 03, 2014 at 07:09:07AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 23, 2014 at 09:23:53PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > When trying to dump objects, we have to treat multi-block objects
> > differently to single block objects. Separate out the code paths for
> > single block vs multi-block objects so we can add a separate path
> > for multi-block objects.
> 
> Looks good, but two minor style nitpicks below.
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> 
> >  static int
> > +process_multi_fsb_objects(
> > +   xfs_dfiloff_t   o,
> > +   xfs_dfsbno_t    s,
> > +   xfs_dfilblks_t  c,
> > +   typnm_t         btype,
> > +   xfs_dfiloff_t   last)
> > +{
> > +   if (btype != TYP_DIR2) {
> > +           print_warning("bad type for multi-fsb object %d", btype);
> > +           return -EINVAL;
> > +   }
> > +
> > +   return process_single_fsb_objects(o, s, c, btype, last);
> 
> I'd prefer a switch with a default statement for the unknown type here,
> as that leads to nicer extensibility.

Ok, I'll fix that up.

> > +           /* single filesystem block objects are trivial to handle */
> > +           if (btype != TYP_DIR2 || mp->m_dirblkfsbs == 1) {
> > +                   error = process_single_fsb_objects(o, s, c, btype, 
> > last);
> > +                   if (error)
> >                             return 0;
> > +                   continue;
> >             }
> > +
> > +           /* multi-extent directory blocks */
> > +           error = process_multi_fsb_objects(o, s, c, btype, last);
> > +           if (error)
> > +                   return 0;
> 
> An if / else would look a little more obvious here, not that it really
> matters all that much.

Easy enough to do.

Thanks for the reviews, Christoph.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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