Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
On Tue, 2007-09-25 at 21:15 +0530, Aneesh Kumar K.V wrote: > > Dave Kleikamp wrote: > > On Tue, 2007-09-25 at 14:33 +0530, Aneesh Kumar K.V wrote: > >> ext4 file system layout contain different split members > >> like bg_block_bitmap and bg_block_bitmap_hi. Introduce > >> data type le32_t and le16_t to be used as the type of > >> these split members. This prevents these members frome > >> being accessed directly. Accesing them directly gives > >> a compiler warning. This helps in catching some BUGS > >> due to direct partial access of these split fields. > > > > Ugh. I don't like this typedef at all. It just makes the data type more > > confusing. > > > Confusing enough to make people look at them more carefully. > > > > > > > Why not just change the name of bg_block_bitmap to something like > > bg_block_bitmap_lo or _bg_block_bitmap? It should be clear from the > > name that it shouldn't be used without careful consideration. > > > > even if we rename the variables, I guess we would like to have helper > functions > for accessing these values. That would mean the code is finally going to look > more > or less the same except the typedef. I see that as a good thing. The typedef is the part I don't like. > But the typedef actually save us from serious > misuse of these variables. You could just as easily write code that misuses bg_block_bitmap->value, as you could bg_block_bitmap_lo. I actually think using a name that describes that it's only half-a-value is more clear. Shaggy -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
Dave Kleikamp wrote: On Tue, 2007-09-25 at 14:33 +0530, Aneesh Kumar K.V wrote: ext4 file system layout contain different split members like bg_block_bitmap and bg_block_bitmap_hi. Introduce data type le32_t and le16_t to be used as the type of these split members. This prevents these members frome being accessed directly. Accesing them directly gives a compiler warning. This helps in catching some BUGS due to direct partial access of these split fields. Ugh. I don't like this typedef at all. It just makes the data type more confusing. Confusing enough to make people look at them more carefully. Why not just change the name of bg_block_bitmap to something like bg_block_bitmap_lo or _bg_block_bitmap? It should be clear from the name that it shouldn't be used without careful consideration. even if we rename the variables, I guess we would like to have helper functions for accessing these values. That would mean the code is finally going to look more or less the same except the typedef. But the typedef actually save us from serious misuse of these variables. -aneesh - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
On Tue, 2007-09-25 at 14:33 +0530, Aneesh Kumar K.V wrote:
> ext4 file system layout contain different split members
> like bg_block_bitmap and bg_block_bitmap_hi. Introduce
> data type le32_t and le16_t to be used as the type of
> these split members. This prevents these members frome
> being accessed directly. Accesing them directly gives
> a compiler warning. This helps in catching some BUGS
> due to direct partial access of these split fields.
Ugh. I don't like this typedef at all. It just makes the data type more
confusing.
Why not just change the name of bg_block_bitmap to something like
bg_block_bitmap_lo or _bg_block_bitmap? It should be clear from the
name that it shouldn't be used without careful consideration.
> Signed-off-by: Aneesh Kumar K.V <[EMAIL PROTECTED]>
> ---
> fs/ext4/super.c |8
> include/linux/ext4_fs.h |4 ++--
> include/linux/ext4_fs_i.h |4
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d035653..c8f8e4d 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -70,9 +70,9 @@ static void ext4_write_super_lockfs(struct super_block *sb);
> ext4_fsblk_t ext4_block_bitmap(struct super_block *sb,
> struct ext4_group_desc *bg)
> {
> - return le32_to_cpu(bg->bg_block_bitmap) |
> + return le32_to_cpu(bg->bg_block_bitmap.value) |
> (EXT4_DESC_SIZE(sb) >= EXT4_MIN_DESC_SIZE_64BIT ?
> - (ext4_fsblk_t)le32_to_cpu(bg->bg_block_bitmap_hi) << 32 : 0);
> + (ext4_fsblk_t)le32_to_cpu(bg->bg_block_bitmap_hi.value) << 32
> : 0);
> }
>
> ext4_fsblk_t ext4_inode_bitmap(struct super_block *sb,
> @@ -94,9 +94,9 @@ ext4_fsblk_t ext4_inode_table(struct super_block *sb,
> void ext4_block_bitmap_set(struct super_block *sb,
> struct ext4_group_desc *bg, ext4_fsblk_t blk)
> {
> - bg->bg_block_bitmap = cpu_to_le32((u32)blk);
> + bg->bg_block_bitmap.value = cpu_to_le32((u32)blk);
> if (EXT4_DESC_SIZE(sb) >= EXT4_MIN_DESC_SIZE_64BIT)
> - bg->bg_block_bitmap_hi = cpu_to_le32(blk >> 32);
> + bg->bg_block_bitmap_hi.value = cpu_to_le32(blk >> 32);
> }
>
> void ext4_inode_bitmap_set(struct super_block *sb,
> diff --git a/include/linux/ext4_fs.h b/include/linux/ext4_fs.h
> index 046a6a7..4494f8e 100644
> --- a/include/linux/ext4_fs.h
> +++ b/include/linux/ext4_fs.h
> @@ -150,7 +150,7 @@ struct ext4_allocation_request {
> */
> struct ext4_group_desc
> {
> - __le32 bg_block_bitmap;/* Blocks bitmap block */
> + le32_t bg_block_bitmap;/* Blocks bitmap block */
> __le32 bg_inode_bitmap;/* Inodes bitmap block */
> __le32 bg_inode_table; /* Inodes table block */
> __le16 bg_free_blocks_count; /* Free blocks count */
> @@ -160,7 +160,7 @@ struct ext4_group_desc
> __u32 bg_reserved[2]; /* Likely block/inode bitmap checksum */
> __le16 bg_itable_unused; /* Unused inodes count */
> __le16 bg_checksum;/* crc16(sb_uuid+group+desc) */
> - __le32 bg_block_bitmap_hi; /* Blocks bitmap block MSB */
> + le32_t bg_block_bitmap_hi; /* Blocks bitmap block MSB */
> __le32 bg_inode_bitmap_hi; /* Inodes bitmap block MSB */
> __le32 bg_inode_table_hi; /* Inodes table block MSB */
> };
> diff --git a/include/linux/ext4_fs_i.h b/include/linux/ext4_fs_i.h
> index 22ba80e..d7eb271 100644
> --- a/include/linux/ext4_fs_i.h
> +++ b/include/linux/ext4_fs_i.h
> @@ -27,6 +27,10 @@ typedef int ext4_grpblk_t;
> /* data type for filesystem-wide blocks number */
> typedef unsigned long long ext4_fsblk_t;
>
> +/* data type to carry split 64 and 48 bits */
> +typedef struct { __le16 value; } le16_t;
> +typedef struct { __le32 value; } le32_t;
> +
> struct ext4_reserve_window {
> ext4_fsblk_t_rsv_start; /* First byte reserved */
> ext4_fsblk_t_rsv_end; /* Last byte reserved or 0 */
--
David Kleikamp
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
Andreas Dilger wrote:
On Sep 25, 2007 14:41 +0530, Aneesh Kumar K.V wrote:
The patches are on top of patch queue. I haven't touched
the uid and gid of ext4_inode. Do you think i should
change that too ?
You can add a Signed-off-by: Andreas Dilger <[EMAIL PROTECTED]>
to those patches. As I suspected there were a number of bugs hiding
in there. I would also change the uid and gid fields, even if we
don't suspect any problems now.
the primary reason for me not looking at uid gid and file_acl fields are
a) we can mount with option nouid32 and that will look at only the low 16
bits
b) same is true with the gid fields
c) Also i_file_acl. If the creater os is HURD we look at only the low 32 bits.
That means all the pace where we access these fields we have conditional access
That makes it less error prone.
with the changes the code will some what as below
if(!(test_opt (inode->i_sb, NO_UID32))) {
- inode->i_uid |= le16_to_cpu(raw_inode->i_uid_high) << 16;
- inode->i_gid |= le16_to_cpu(raw_inode->i_gid_high) << 16;
+ inode->i_uid = ext4_get_i_uid(raw_inode);
+ inode->i_gid = ext4_get_i_gid(raw_inode);
+ } else {
+ inode->i_uid = ext4_get_i_uid_low(raw_inode);
+ inode->i_gid = ext4_get_i_gid_low(raw_inode);
}
-aneesh
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
On Sep 25, 2007 14:41 +0530, Aneesh Kumar K.V wrote: > The patches are on top of patch queue. I haven't touched > the uid and gid of ext4_inode. Do you think i should > change that too ? You can add a Signed-off-by: Andreas Dilger <[EMAIL PROTECTED]> to those patches. As I suspected there were a number of bugs hiding in there. I would also change the uid and gid fields, even if we don't suspect any problems now. > 7/7 is not the part of the series. But it is important. > Should i send it as a separate patch for the 2.6.24 ? Probably yes. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ext4: Introduce le32_t and le16_t
Andreas, The patches are on top of patch queue. I haven't touched the uid and gid of ext4_inode. Do you think i should change that too ? 7/7 is not the part of the series. But it is important. Should i send it as a separate patch for the 2.6.24 ? -aneesh - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
