OK, I've addressed all your concerns and here is a new version of the patch. With it, the delta-size of the compiled ext2.mod against a completely unpatched one is just 148 bytes.
El vie, 04-07-2008 a las 20:57 +0200, Robert Millan escribió: > On Fri, Jul 04, 2008 at 04:45:02PM +0200, Javier Martín wrote: > > > > By the way, I'm already using SVN (and thus svn diff) for this patch. Is > > that right? Was the migration completed already? > > Yep. Wonderful! I was sick of jumping through hoops with cvs diff. > I'd suggest making the "RW compatible" etc notes a bit more ellaborate to make > it clear what they mean (I'm confused myself). Done, though now I might have over-elaborated > > +/* The set of back-incompatible features this driver DOES support. Add (OR) > > + * flags here as the related features are implemented into the driver */ > > +#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE ) > > I suppose we'll want to have EXT4_FEATURE_INCOMPAT_EXTENTS here, now that it's > been implemented (Bean just sent a patch, which will probably be merged > first). Done too and checked that ext4 filesystems w/o other incompatible features like 64BIT are now recognized (though I did not check reading since I haven't yet applied Bean's patch to my tree). > > +/* The set of back-incompatible features this driver DOES NOT support but > > are > > + * ignored for some hackish reason. Flags here should be here > > _temporarily_! > > + * Remember that INCOMPAT_* features are so for a reason! */ > > +#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER ) > > Instead of this can we have an explanation of what > EXT3_FEATURE_INCOMPAT_RECOVER > is doing here? I think the reason was that our code for this feature wasn't > as mature as it should be, and it appears that not handling it brings better > results in the short term. Done - explained why RECOVER is ignored even though it's "incompatible" > Since we know which one applies, why not tell grub_error about it? We could > leave the "not an ext2 filesystem" call unmodified and add another one for > this particular error. > I may have overstepped a bit, but I've thought it more sensible to replace all "goto fail;"s for calls to a new macro MOUNT_FAIL taking a string argument which is saved in the new variable err_msg, and then jumps to fail which shows _that_ message instead of the old one. Then, I wrote informative messages for each error condition instead of just "not an ext2 filesystem".
Index: fs/ext2.c
===================================================================
--- fs/ext2.c (revisión: 1691)
+++ fs/ext2.c (copia de trabajo)
@@ -71,8 +71,53 @@
? EXT2_GOOD_OLD_INODE_SIZE \
: grub_le_to_cpu16 (data->sblock.inode_size))
-#define EXT3_FEATURE_COMPAT_HAS_JOURNAL 0x0004
+/* Superblock filesystem feature flags (RW compatible)
+ * A filesystem with any of these enabled can be read and written by a driver
+ * that does not understand them without causing metadata/data corruption */
+#define EXT2_FEATURE_COMPAT_DIR_PREALLOC 0x0001
+#define EXT2_FEATURE_COMPAT_IMAGIC_INODES 0x0002
+#define EXT3_FEATURE_COMPAT_HAS_JOURNAL 0x0004
+#define EXT2_FEATURE_COMPAT_EXT_ATTR 0x0008
+#define EXT2_FEATURE_COMPAT_RESIZE_INODE 0x0010
+#define EXT2_FEATURE_COMPAT_DIR_INDEX 0x0020
+/* Superblock filesystem feature flags (RO compatible)
+ * A filesystem with any of these enabled can be safely read by a driver that
+ * does not understand them, but should not be written to, usually because
+ * additional metadata is required */
+#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001
+#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE 0x0002
+#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR 0x0004
+#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010
+#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020
+#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040
+/* Superblock filesystem feature flags (back-incompatible)
+ * A filesystem with any of these enabled should not be attempted to be read
+ * by a driver that does not understand them, since they usually indicate
+ * metadata format changes that might confuse the reader. */
+#define EXT2_FEATURE_INCOMPAT_COMPRESSION 0x0001
+#define EXT2_FEATURE_INCOMPAT_FILETYPE 0x0002
+#define EXT3_FEATURE_INCOMPAT_RECOVER 0x0004 /* Needs recovery */
+#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV 0x0008 /* Volume is journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG 0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 /* Extents used */
+#define EXT4_FEATURE_INCOMPAT_64BIT 0x0080
+#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200
+/* The set of back-incompatible features this driver DOES support. Add (OR)
+ * flags here as the related features are implemented into the driver */
+#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE \
+ | EXT4_FEATURE_INCOMPAT_EXTENTS )
+/* List of rationales for the ignored "incompatible" features:
+ * needs_recovery: Not really back-incompatible - was added as such to forbid
+ * ext2 drivers from mounting an ext3 volume with a dirty
+ * journal because they will ignore the journal, but the next
+ * ext3 driver to mount the volume will find the journal and
+ * replay it, potentially corrupting the metadata written by
+ * the ext2 drivers
+ */
+#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )
+
+
#define EXT3_JOURNAL_MAGIC_NUMBER 0xc03b3998U
#define EXT3_JOURNAL_DESCRIPTOR_BLOCK 1
@@ -375,10 +420,12 @@
return 0;
}
+#define EXT2_DRIVER_MOUNT_FAIL(message) { err_msg = (message); goto fail; }
static struct grub_ext2_data *
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ const char *err_msg = 0;
data = grub_malloc (sizeof (struct grub_ext2_data));
if (!data)
@@ -388,12 +435,18 @@
grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
(char *) &data->sblock);
if (grub_errno)
- goto fail;
+ EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
/* Make sure this is an ext2 filesystem. */
if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
- goto fail;
+ EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem (superblock magic mismatch)")
+ /* Check the FS doesn't have feature bits enabled that we don't support. */
+ if (grub_le_to_cpu32 (data->sblock.feature_incompat)
+ & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+ EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features")
+
+
data->disk = disk;
data->diropen.data = data;
@@ -404,12 +457,14 @@
grub_ext2_read_inode (data, 2, data->inode);
if (grub_errno)
- goto fail;
+ EXT2_DRIVER_MOUNT_FAIL("could not read the root directory inode")
return data;
fail:
- grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+ if (!err_msg)
+ err_msg = "DEBUG: mount failed but no error message supplied!";
+ grub_error (GRUB_ERR_BAD_FS, err_msg);
grub_free (data);
return 0;
}
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
