On Feb 06, 2007 16:12 +0100, Johann Lombardi wrote:
> > + if (sbi->s_inode_size > EXT3_GOOD_OLD_INODE_SIZE) {
> > + EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
>
> Maybe EXT3_SB(sb)-> could be replaced by sbi-> here and in the lines below.
Yes, this should definitely be done. It also increases clarity between
sbi->s_want_extra_isize and es->s_want_extra_isize.
> > + if (EXT3_SB(sb)->s_want_extra_isize <
> > + le32_to_cpu(es->s_min_extra_isize))
> ^^
> > + EXT3_SB(sb)->s_want_extra_isize =
> > + le32_to_cpu(es->s_min_extra_isize);
> ^^
> Since es->s_{min,want}_extra_isize are both __u16 (BTW, shouldn't it be
> __le16?), I think you should use le16_to_cpu() instead of le32_to_cpu().
You are right - this works fine on little endian systems, but fails on
big endian systems where you will get the other half of the word.
This has been a bug in several places already, and I wonder if the
le*_to_cpu() and cpu_to_le*() macros shouldn't do some type checking
instead of just casting the variable to the specified type?
The only problem is if casting constants it would be a bit of a pain
to have to cast them explicitly, though we could have something like:
#define le16_to_cpu(var) (__builtin_constant(var) || !typecheck(__u16, var) ? \
__constant_cpu_to_le16(var) : __le16_to_cpu(var))
The only question is whether "typecheck" adds extra variables on the stack
or if the compiler will always optimize them away.
> > + /* Check if enough inode space is available */
> > + if (EXT3_GOOD_OLD_INODE_SIZE + EXT3_SB(sb)->s_want_extra_isize >
> > + sbi->s_inode_size) {
> > + EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
> > + printk(KERN_INFO "EXT3-fs: required extra inode space not"
> > + "available.\n");
> > + }
>
> If the inode size is EXT3_GOOD_OLD_INODE_SIZE, sbi->s_want_extra_isize won't
> be initialized. However, it should not be an issue because the ext3_sb_info
> is set to zero in ext3_fill_super().
So I'm not sure I understand if you have an objection or if this is just a
comment. sbi->s_want_extra_isize will be zero and it is not possible for
sbi->s_inode_size < EXT3_GOOD_OLD_INODE_SIZE so this case won't be hit.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
