Hi Christoph.
On Mon, Jan 14, 2019 at 05:56:17PM +0100, Christoph Hellwig wrote:
> > +int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > + u64 phys, u64 len, u32 flags)
>
> Any reason this function isn't in inode.c next to the caller and marked
> static?
>
No reason other than to keep it close to its peer fiemap_fill_user_extent(), I
honestly do prefer to keep both together than in separated files. But, I'm up
to move it to fs/inode.c if required.
> Otherwise looks fine except for the additional sanity checking pointed
> out by Darrick.
Working on that.
>
> > + /* only count the extents */
> > + if (fieinfo->fi_extents_max == 0) {
> > + fieinfo->fi_extents_mapped++;
> > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
>
> Maybe do a 'goto out' here?
>
> > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
>
> And reuse this return. Bonus points for using a good old
> if here:
>
> if (flags & FIEMAP_EXTENT_LAST)
> return 1;
> return 0;
Ok, will be in the new version, thanks for the review :)
--
Carlos
