Re: [PATCH 16/21] implement generic xfs_btree_lshift

[Christoph Hellwig <hch@lst.de>]

> > +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.

> This can be moved inside the branch it is used in.

Done.

> Move the XFS_BTREE_STATS_ADD() above the comment to match the rshift
> code.

Done.