All of lore.kernel.org
 help / color / mirror / Atom feed
From: Carlos Maiolino <cmaiolino@redhat.com>
To: Hou Tao <houtao1@huawei.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: remove the magic numbers in xfs_btree_block-related len macros
Date: Thu, 23 Jun 2016 12:21:37 +0200	[thread overview]
Message-ID: <20160623102137.GH27894@redhat.com> (raw)
In-Reply-To: <1466651420-126472-1-git-send-email-houtao1@huawei.com>

On Thu, Jun 23, 2016 at 11:10:20AM +0800, Hou Tao wrote:
> replace the magic numbers by offsetof(...) and sizeof(...), and add two
> extra checks on xfs_check_ondisk_structs()
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/xfs/libxfs/xfs_format.h | 66 ++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_ondisk.h        |  2 ++
>  2 files changed, 43 insertions(+), 25 deletions(-)
> 

Particularly I liked the idea of defining the short and long block structures
outside of xfs_btree_block and removing magic numbers is a good thing too.

you can consider it

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index dc97eb21..d3069be 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1435,41 +1435,57 @@ typedef __be64 xfs_bmbt_ptr_t, xfs_bmdr_ptr_t;
>   * with the crc feature bit, and all accesses to them must be conditional on
>   * that flag.
>   */
> +/* short form block */
> +struct xfs_btree_sblock_part {
> +	__be32		bb_leftsib;
> +	__be32		bb_rightsib;
> +
> +	__be64		bb_blkno;
> +	__be64		bb_lsn;
> +	uuid_t		bb_uuid;
> +	__be32		bb_owner;
> +	__le32		bb_crc;
> +};
> +
> +/* long form block */
> +struct xfs_btree_lblock_part {
> +	__be64		bb_leftsib;
> +	__be64		bb_rightsib;
> +
> +	__be64		bb_blkno;
> +	__be64		bb_lsn;
> +	uuid_t		bb_uuid;
> +	__be64		bb_owner;
> +	__le32		bb_crc;
> +	__be32		bb_pad; /* padding for alignment */
> +};
> +
>  struct xfs_btree_block {
>  	__be32		bb_magic;	/* magic number for block type */
>  	__be16		bb_level;	/* 0 is a leaf */
>  	__be16		bb_numrecs;	/* current # of data records */
>  	union {
> -		struct {
> -			__be32		bb_leftsib;
> -			__be32		bb_rightsib;
> -
> -			__be64		bb_blkno;
> -			__be64		bb_lsn;
> -			uuid_t		bb_uuid;
> -			__be32		bb_owner;
> -			__le32		bb_crc;
> -		} s;			/* short form pointers */
> -		struct	{
> -			__be64		bb_leftsib;
> -			__be64		bb_rightsib;
> -
> -			__be64		bb_blkno;
> -			__be64		bb_lsn;
> -			uuid_t		bb_uuid;
> -			__be64		bb_owner;
> -			__le32		bb_crc;
> -			__be32		bb_pad; /* padding for alignment */
> -		} l;			/* long form pointers */
> +		struct xfs_btree_sblock_part s;
> +		struct xfs_btree_lblock_part l;
>  	} bb_u;				/* rest */
>  };
>  
> -#define XFS_BTREE_SBLOCK_LEN	16	/* size of a short form block */
> -#define XFS_BTREE_LBLOCK_LEN	24	/* size of a long form block */
> +/* size of a short form block */
> +#define XFS_BTREE_SBLOCK_LEN \
> +	(offsetof(struct xfs_btree_block, bb_u) + \
> +	 offsetof(struct xfs_btree_sblock_part, bb_blkno))
> +/* size of a long form block */
> +#define XFS_BTREE_LBLOCK_LEN \
> +	(offsetof(struct xfs_btree_block, bb_u) + \
> +	 offsetof(struct xfs_btree_lblock_part, bb_blkno))
>  
>  /* sizes of CRC enabled btree blocks */
> -#define XFS_BTREE_SBLOCK_CRC_LEN	(XFS_BTREE_SBLOCK_LEN + 40)
> -#define XFS_BTREE_LBLOCK_CRC_LEN	(XFS_BTREE_LBLOCK_LEN + 48)
> +#define XFS_BTREE_SBLOCK_CRC_LEN \
> +	(offsetof(struct xfs_btree_block, bb_u) + \
> +	 sizeof(struct xfs_btree_sblock_part))
> +#define XFS_BTREE_LBLOCK_CRC_LEN \
> +	(offsetof(struct xfs_btree_block, bb_u) + \
> +	 sizeof(struct xfs_btree_lblock_part))
>  
>  #define XFS_BTREE_SBLOCK_CRC_OFF \
>  	offsetof(struct xfs_btree_block, bb_u.s.bb_crc)
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 184c44e..6f06c48 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -34,6 +34,8 @@ xfs_check_ondisk_structs(void)
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_bmbt_key,		8);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_bmbt_rec,		16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_bmdr_block,		4);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_btree_sblock_part,	48);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_btree_lblock_part,	64);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_btree_block,		72);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_dinode,		176);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_disk_dquot,		104);
> -- 
> 2.5.5
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

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

  reply	other threads:[~2016-06-23 10:21 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-23  3:10 [PATCH] xfs: remove the magic numbers in xfs_btree_block-related len macros Hou Tao
2016-06-23 10:21 ` Carlos Maiolino [this message]
2016-06-24  6:43   ` Hou Tao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160623102137.GH27894@redhat.com \
    --to=cmaiolino@redhat.com \
    --cc=houtao1@huawei.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.