xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 10 Feb 2014 14:31:40 +1100
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52F83907.9070004@xxxxxxxxxxx>
References: <52F83630.4020008@xxxxxxxxxx> <52F83907.9070004@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> ---
> 
> 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@xxxxxxxxxxxxx

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