On Sun, 18 Jan 2009, Stanislav Sedov wrote:

Author: stas
Date: Sun Jan 18 14:04:56 2009
New Revision: 187395
URL: http://svn.freebsd.org/changeset/base/187395

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.lyash...@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 "u.ext2.sb".  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) ? \
%                                EXT2_GOOD_OLD_INODE_SIZE : \
%                                (s)->s_inode_size)
% #define EXT2_FIRST_INO(s)     (((s)->s_rev_level == EXT2_GOOD_OLD_REV) ? \
%                                EXT2_GOOD_OLD_FIRST_INO : \
%                                (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.

Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to