svn commit: r187395 - head/sys/gnu/fs/ext2fs

Bruce Evans brde at
Mon Jan 19 03:45:05 PST 2009

On Sun, 18 Jan 2009, Stanislav Sedov wrote:

> Author: stas
> Date: Sun Jan 18 14:04:56 2009
> New Revision: 187395
> URL:
> Log:
>  - Obtain inode sizes and location of the first inode based on the contents
>    of superblock rather than using hardcoded values. This fixes ext2fs on
>    filesystems with inode sized other than 128.
>  Submitted by:	Alex Lyashkov <Alexey.Lyashkov at Sun.COM> (based on)
>  MFC after:	2 weeks

This was not suitable for committing verbatim.

> Modified: head/sys/gnu/fs/ext2fs/ext2_fs.h
> ==============================================================================
> --- head/sys/gnu/fs/ext2fs/ext2_fs.h	Sun Jan 18 13:04:38 2009	(r187394)
> +++ head/sys/gnu/fs/ext2fs/ext2_fs.h	Sun Jan 18 14:04:56 2009	(r187395)
> @@ -150,8 +150,8 @@
> #else /* !notyet */
> #define	EXT2_INODES_PER_BLOCK(s)	((s)->s_inodes_per_block)
> /* Should be sizeof(struct ext2_inode): */
> -#define EXT2_INODE_SIZE			128
> -#define EXT2_FIRST_INO			11
> +#define EXT2_INODE_SIZE(s)		((s)->s_inode_size)
> +#define EXT2_FIRST_INO(s)		((s)->s_first_inode)
> #endif /* notyet */

Please read the code before changing parts of it.

The comment is now just wrong.  One value that EXT2_INODE_SIZE certainly
should _not_ be is the fixed size of our data structure, since we are
now trying to support a variable size and even have a parameter to do that.

The ifdef is now more bogus than before.  The "!notyet" part now
essentially duplicates the old unchanged "notyet" part, except for
having bugs not present in the old version.  This ifdef is a FreeBSD
hack which should have been removed.  Linux ext2fs just has the
"notyet" part.  Here is complete old ifdef:

% #ifdef notyet
% #ifdef __KERNEL__
% #define	EXT2_ADDR_PER_BLOCK_BITS(s)	((s)->u.ext2_sb.s_addr_per_block_bits)
% #define EXT2_INODE_SIZE(s)		((s)->u.ext2_sb.s_inode_size)
% #define EXT2_FIRST_INO(s)		((s)->u.ext2_sb.s_first_ino)

This is literally the same as in Linux.  It cannot be used directly, now
for only minor reasons:
- FreeBSD doesn't have "".  After deleting "u.ext2_sb." from
   the above, it seems to be right.
- this was already done in the !notyet part for the 1st macro
- your change does this for the other 2 macros, but this is not enough --
   see below.

% #else
% #define EXT2_INODE_SIZE(s)	(((s)->s_rev_level == EXT2_GOOD_OLD_REV) ? \
% 				 (s)->s_inode_size)
% #define EXT2_FIRST_INO(s)	(((s)->s_rev_level == EXT2_GOOD_OLD_REV) ? \
% 				 (s)->s_first_ino)

s_inode_size and s_first_ino cannot be used directly because they
aren't supported by EXT2_GOOD_OLD_REV file systems.  The conversion
is done in the above macros for the !KERNEL case.  It is done during
superblock initialization in Linux ext2fs (at least the in-core copy
of the superblock is written to).  It is not done in FreeBSD ext2fs.
Thus support for EXT2_GOOD_OLD_REV file systems has been broken.

The change seems to be correct except for the above, and 1 other major
style bug.


More information about the svn-src-head mailing list