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