Re: [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs

[Dave Chinner <david@fromorbit.com>]

On Sun, Feb 09, 2014 at 08:27:19PM -0600, Eric Sandeen wrote:
> Many/most callers of xfs_verify_cksum() pass bp->b_addr and
> BBTOB(bp->b_length) as the first 2 args.  Add a helper
> which can just accept the bp and the crc offset, and work
> it out on its own, for brevity.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> I'm not wedded to this; seems helpful and cleaner, but
> if there's a reason not to, *shrug*
> 
>  fs/xfs/xfs_alloc.c          |    7 +++----
>  fs/xfs/xfs_attr_leaf.c      |    3 +--
>  fs/xfs/xfs_btree.c          |    8 ++++----
>  fs/xfs/xfs_cksum.h          |    7 +++++++
>  fs/xfs/xfs_da_btree.c       |    3 +--
>  fs/xfs/xfs_dir2_block.c     |    3 +--
>  fs/xfs/xfs_dir2_data.c      |    3 +--
>  fs/xfs/xfs_dir2_leaf.c      |    3 +--
>  fs/xfs/xfs_dir2_node.c      |    3 +--
>  fs/xfs/xfs_ialloc.c         |    4 ++--
>  fs/xfs/xfs_symlink_remote.c |    2 +-
>  11 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9eab2df..ab33714 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -485,8 +485,7 @@ xfs_agfl_read_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return;
>  
> -	agfl_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -				   offsetof(struct xfs_agfl, agfl_crc));
> +	agfl_ok = xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc));
					   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
vs

> +          !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
> 				       ^^^^^^^^^^^^^^^^^^^^^^

I think that we also be consistent with the way we specify
the offset of the CRC field. Some of the code uses a #define, and
some doesn't. My preference would be to use the #define format
everywhere as it makes the code briefer and easier to read and it
gets rid of the long lines and wrapping that is done in places.

> diff --git a/fs/xfs/xfs_cksum.h b/fs/xfs/xfs_cksum.h
> index fad1676..f605d64 100644
> --- a/fs/xfs/xfs_cksum.h
> +++ b/fs/xfs/xfs_cksum.h
> @@ -60,4 +60,11 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
>  	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
>  }
>  
> +static inline int
> +xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
> +{
> +	return xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> +				cksum_offset);
> +}
> +

This introduces a dependency between xfs_buf.h and xfs_cksum.h.
i.e. if we include xfs_cksum.h we now have to include xfs_buf.h
before it.

Even though it means we'll have to juggle header files in quite a
few files, I'd prefer that we swap the dependency order as
xfs_cksum.h is shared with userspace and used in places that don't
necessarily know (or should know) what an xfs_buf is....

Otherwise it's a good cleanup.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs