On Tue, 2011-09-06 at 11:27 -0400, Christoph Hellwig wrote:
> > > + xfs_extnum_t lastx; /* last useful extent number */
> > > + int eof; /* we've hit the end of extents */
> > > + int n = 0; /* current extent index */
> > > + int error = 0;
> > > +
> > > + ASSERT(*nmap >= 1);
> > > + ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
> > > + ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
> > > +
> >
> > Rearrange the following test to use the pattern (assigning error)
> > used in xfs_bmapi_read().
>
> Hmm - given that error is used as a boolean there I don't actually like
> that pattern very much as error is generally used to hold an errno
> value.
My main thought was "make them consistent." But
looking at it again I agree that "error" is not
the right name. It doesn't really need changing
but could be beautified in some later patch.
> >
> > > + if (unlikely(XFS_TEST_ERROR(
> > > + (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> > > + XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> >
> > Are you certain that XFS_DINODE_FMT_LOCAL is not possible here?
> > I tried to trace it back but I'm still not sure. The transaction
> > pointer passed is null, so it would have tripped an assertion
> > in the previous code. (A simple explanation would reassure me.)
>
> We can't hit it because we do not support the local format for regular
> files at all, and we do not support delayed allocations for anything
> but regular files.
OK. That's another thing I didn't realize about XFS...
-Alex
|