xfs
[Top] [All Lists]

Re: [PATCH 16/21] implement generic xfs_btree_lshift

To: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 16/21] implement generic xfs_btree_lshift
From: Christoph Hellwig <hch@xxxxxx>
Date: Fri, 1 Aug 2008 21:52:49 +0200
In-reply-to: <20080730062422.GQ13395@disturbed>
References: <20080729193132.GQ19104@xxxxxx> <20080730062422.GQ13395@disturbed>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.3.28i
> > +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.


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