Re: [PATCH 16/21] implement generic xfs_btree_lshift

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 01, 2008 at 09:52:49PM +0200, Christoph Hellwig wrote:
> > > +xfs_btree_copy_ptrs(
> > > +	struct xfs_btree_cur	*cur,
> > > +	union xfs_btree_ptr	*src_ptr,
> > > +	union xfs_btree_ptr	*dst_ptr,
> > > +	int			numptrs)
> > > +{
> > > +	ASSERT(numptrs > 0);
> > > +
> > > +	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
> > > +		memcpy(dst_ptr, src_ptr, numptrs * sizeof(__be64));
> > > +	else
> > > +		memcpy(dst_ptr, src_ptr, numptrs * sizeof(__be32));
> > > +}
> > 
> > These should really use memmove, not memcpy. There is no guarantee
> > the source and destination do not overlap.
> > 
> > At minimum, we need comments to say this must only be used to
> > copy between blocks, and xfs_btree_move_ptrs() must be used to
> > copy within a block. I note the original patchset of mine
> > commented on this distinction when defining the ->move_* and
> > ->copy_* operations.
> > 
> > FWIW, that also helps explain why they have different interfaces...
> 
> There were some comments in the pre-walkthru cleanup version but they
> were already lost in that patch.  But yes, adding some comments makes
> sense.  Or moving back to single one that unlike your very first
> version always passes src and dst pointers and always uses memmove.

It might make sense to go back to a single implementation,
though at the time I did it it made sense to split the move/copy
operations because it made both cases simpler. Seeing as you've
stuck more closely to the original structure of the code, the
distinction is not as great as so it might be best to go back to a
single memmove based interface.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com