public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs: remove the magic numbers in xfs_btree_block-related len macros
@ 2016-06-23  3:10 Hou Tao
  2016-06-23 10:21 ` Carlos Maiolino
  0 siblings, 1 reply; 3+ messages in thread
From: Hou Tao @ 2016-06-23  3:10 UTC (permalink / raw)
  To: xfs

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(-)

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: remove the magic numbers in xfs_btree_block-related len macros
  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
  2016-06-24  6:43   ` Hou Tao
  0 siblings, 1 reply; 3+ messages in thread
From: Carlos Maiolino @ 2016-06-23 10:21 UTC (permalink / raw)
  To: Hou Tao; +Cc: xfs

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: remove the magic numbers in xfs_btree_block-related len macros
  2016-06-23 10:21 ` Carlos Maiolino
@ 2016-06-24  6:43   ` Hou Tao
  0 siblings, 0 replies; 3+ messages in thread
From: Hou Tao @ 2016-06-24  6:43 UTC (permalink / raw)
  To: Carlos Maiolino; +Cc: xfs



On 2016/6/23 18:21, Carlos Maiolino wrote:
> 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
> 
I had tried to do more than just move two structs out of struct xfs_btree_block,
but the modification would be huge, so i just keep it simple.

> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
Thanks for your review.


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

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-06-24  6:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2016-06-24  6:43   ` Hou Tao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox