Re: grub-probe detects ext4 wronly as ext2
Am Mittwoch, den 04.02.2009, 14:08 +0100 schrieb Javier Martín: Well, I am happy to post a diff of the patch against current SVN head (r1973). I have personally confirmed (in a VM) that it: 1) Still builds (and even runs! ^^) 2) Works with existing ext2/3 file systems (I haven't checked ext4 FSs but the extents bit is marked as supported, so it should work) 3) Correctly rejects journal devices, which will then appear as unknown filesystem when accessed. FLEX_BG needs to be added to the list of ignored flags. As Robert already said in his last reply to this thread [0] const char *local_error = 0; Please use NULL. +EXT2_DRIVER_MOUNT_FAIL(0); I share his opinion that this isn't needed. If you fix this and write a changelog then I commit this. [0] http://lists.gnu.org/archive/html/grub-devel/2008-08/msg00645.html -- Felix Zielcke ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El sáb, 07-02-2009 a las 20:30 +0100, Felix Zielcke escribió: Am Mittwoch, den 04.02.2009, 14:08 +0100 schrieb Javier Martín: Well, I am happy to post a diff of the patch against current SVN head (r1973). I have personally confirmed (in a VM) that it: 1) Still builds (and even runs! ^^) 2) Works with existing ext2/3 file systems (I haven't checked ext4 FSs but the extents bit is marked as supported, so it should work) 3) Correctly rejects journal devices, which will then appear as unknown filesystem when accessed. FLEX_BG needs to be added to the list of ignored flags. As Robert already said in his last reply to this thread [0] const char *local_error = 0; Please use NULL. +EXT2_DRIVER_MOUNT_FAIL(0); I share his opinion that this isn't needed. If you fix this and write a changelog then I commit this. [0] http://lists.gnu.org/archive/html/grub-devel/2008-08/msg00645.html Oops... I was going to send a new version of the patch with those fixed, but when doing a svn up so that it would be against HEAD, I've noticed that Robert has just integrated a much cleaner version without the macro and local_error thingies. Well, the only thing left to do is adding flex_bg - here goes the patch. It also clarifies a comment and corrects those added in my original patch and Robert's cleaned-up version that don't end with . */ as they should. -- Lazy, Oblivious, Rational Disaster -- Habbit BTW: Robert, you're having a total mailing spree today! What's it been, 30 posts? Evolution nearly choked, and my spam filter was about to ban you as mass mailing - possible spam ;) Index: fs/ext2.c === --- fs/ext2.c (revision 1977) +++ fs/ext2.c (working copy) @@ -73,7 +73,7 @@ /* 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 */ + * 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 @@ -83,7 +83,7 @@ /* 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 */ + * 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 @@ -93,7 +93,7 @@ /* 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. */ + * 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 */ @@ -104,17 +104,17 @@ #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 */ + * flags here as the related features are implemented into the driver. */ #define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE \ - | EXT4_FEATURE_INCOMPAT_EXTENTS ) + | EXT4_FEATURE_INCOMPAT_EXTENTS \ + | EXT4_FEATURE_INCOMPAT_FLEX_BG ) /* 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 - */ + * the ext2 drivers. Safe to ignore for this RO driver. */ #define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER ) 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
Re: grub-probe detects ext4 wronly as ext2
On Sun, Feb 08, 2009 at 12:54:29AM +0100, Javier Martín wrote: Well, the only thing left to do is adding flex_bg - here goes the patch. It also clarifies a comment and corrects those added in my original patch and Robert's cleaned-up version that don't end with . */ as they should. Committed, thanks. BTW: Robert, you're having a total mailing spree today! What's it been, 30 posts? Evolution nearly choked, and my spam filter was about to ban you as mass mailing - possible spam ;) Yeah I guess I should check the list more often. It's been a while... -- Robert Millan The DRM opt-in fallacy: Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mié, 04-02-2009 a las 08:41 +0100, Felix Zielcke escribió:
Am Montag, den 11.08.2008, 16:14 +0200 schrieb Javier Martín:
Hi there,
After reading Felix's reply I've once again found your post and
implemented your request, so here is a new version of the patch
(version 6). Sorry for missing your message in the first instance...
u_u
I'd like to bring this up again.
Now with that journal_dev bug (See [0] or [1]) it would be really nice
to have this commited.
[0] http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00018.html
[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=502333
Well, I am happy to post a diff of the patch against current SVN head
(r1973). I have personally confirmed (in a VM) that it:
1) Still builds (and even runs! ^^)
2) Works with existing ext2/3 file systems (I haven't checked ext4 FSs
but the extents bit is marked as supported, so it should work)
3) Correctly rejects journal devices, which will then appear as unknown
filesystem when accessed.
Index: fs/ext2.c
===
--- fs/ext2.c (revisión: 1973)
+++ 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
@@ -486,10 +531,12 @@
return 0;
}
+#define EXT2_DRIVER_MOUNT_FAIL(message) { local_error = (message); goto fail; }
static struct grub_ext2_data *
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ const char *local_error = 0;
data = grub_malloc (sizeof (struct grub_ext2_data));
if (!data)
@@ -498,13 +545,19 @@
/* Read the superblock. */
grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
(char *) data-sblock);
- if (grub_errno)
-goto fail;
+ if (grub_errno != GRUB_ERR_NONE)
+EXT2_DRIVER_MOUNT_FAIL(0);
/* 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);
+ /* Check the FS doesn't have feature bits enabled that we don't support. */
+ if (grub_le_to_cpu32 (data-sblock.feature_incompat)
+
Re: grub-probe detects ext4 wronly as ext2
Am Montag, den 11.08.2008, 16:14 +0200 schrieb Javier Martín: Hi there, After reading Felix's reply I've once again found your post and implemented your request, so here is a new version of the patch (version 6). Sorry for missing your message in the first instance... u_u I'd like to bring this up again. Now with that journal_dev bug (See [0] or [1]) it would be really nice to have this commited. [0] http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00018.html [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=502333 -- Felix Zielcke ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Hello there! It's nice to be in town again, though post-vacation
syndrome is hitting me full... Resuming the thread,
El sáb, 30-08-2008 a las 13:17 +0200, Robert Millan escribió:
On Mon, Aug 11, 2008 at 04:14:00PM +0200, Javier Martín wrote:
This overrides the grub_errno and grub_errmsg provided by
grub_disk_read and
replaces them with values that hide the true problem. If there was a
disk
read error, we really want to know about it from the lower layer.
Well, the old version did just the same (even worse, because the message
was generic). What would be the correct path of action here? I mean, how
can we propagate the error messages?
It shouldn't call grub_error().
So in the cases the fail is caused by an underlying error (like I/O)
the code should just return failure and leave the old error in errno
and errmsg?
Yes.
Ok, so that's already done in the previous patch.
static struct grub_ext2_data *
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ const char *local_error = 0;
Please use NULL for pointers.
I don't object at all, but 0 is used throughout the GRUB code to
represent null pointers (see the args struct in any command source
file), so I don't understand the criterion being applied here.
- if (grub_errno)
+ if (grub_errno != GRUB_ERR_NONE)
Why?
1st, because the condition being checked is explicit and thus clearer.
These int-bool C conventions are, even though enshrined by ANSI and
thus pretty standard and reliable, irky at best and due only to the lack
of a proper boolean type. As I think I stated before, the only cases I
use such cast is when checking for null pointers and when using
variables that are explicitly boolean in nature.
2nd, because the new code does not assume that GRUB_ERR_NONE is defined
to zero. Even though this definition will most likely never change, we
might one day decide that -42 is a better value for success.
3rd, because in addition to all that, with any sensible compiler, the
additional binary size cost is nothing at all.
-goto fail;
+EXT2_DRIVER_MOUNT_FAIL(0);
I find very little use in this. I assume it's supposed to simplify things,
but in fact it adds an extra level of indirection for someone who's reading
the code. It provides no runtime improvement, and it's inconsistent with what
we do elsewhere.
It is not meant to provide any runtime improvement, it's just for
consistency: since local_error is already zero, a goto fail would
suffice but I think this uniformity adds readability, not hinders it:
the referenced macro is at the very top of the function, not on a
included header, so the reference is not very time-consuming; and it's
not very complex, so it only needs to be read once. Besides, most
compilers should just optimize the extra assignment away given that the
value of local_error is ct-known for the code paths leading to that
statement and it is not a volatile variable.
+ /* Only call grub_error if the fail was _not_ caused by underlying
errors. */
No need to document this. It's the same deal in a huge amount of routines
throurough the GRUB source.
OK, removed the comment. Maybe a similar comment will be fine over the
macro, though.
That was all, folks! Given that I (think I) addressed your two main
objections, I won't send a new patch right now. I will continue to read
the list this month, but my availability is likely to vary wildly, as I
will be trapped by the ensnaring bureaucracy of the Spanish
universities, scholarships, etc.
-Habbit
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
Re: grub-probe detects ext4 wronly as ext2
Hello list, Am Montag, den 11.08.2008, 16:14 +0200 schrieb Javier Martín: Hi there, After reading Felix's reply I've once again found your post and implemented your request, so here is a new version of the patch (version 6). Sorry for missing your message in the first instance... u_u Any comments about it? I'd really like to have it included but I still think I'm a bit the wrong person to decide if it can be commited now or not ;) -- Felix Zielcke ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Am Montag, den 11.08.2008, 02:35 +0200 schrieb Javier Martín: El mié, 06-08-2008 a las 12:36 +0200, Felix Zielcke escribió: [0] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg8.html [1] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00333.html Thanks for raising the topic again. If it serves any purpose, I'll say that the last patch I sent (version 5) is still valid against the current HEAD (rev. 1798) You're welcome. As you can see in [1] Robert wanted to have it a bit changed. It would be very kind of you, if you'd do that. I doubt the patch gets commited if I change it so it gets accepted and I don't want to have my name on it, it's yours :) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Hi there, After reading Felix's reply I've once again found your post and implemented your request, so here is a new version of the patch (version 6). Sorry for missing your message in the first instance... u_u 2008/7/19 Robert Millan [EMAIL PROTECTED]: On Sat, Jul 05, 2008 at 08:36:13PM +0200, Javier Martín wrote: 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) This overrides the grub_errno and grub_errmsg provided by grub_disk_read and replaces them with values that hide the true problem. If there was a disk read error, we really want to know about it from the lower layer. Well, the old version did just the same (even worse, because the message was generic). What would be the correct path of action here? I mean, how can we propagate the error messages? It shouldn't call grub_error(). So in the cases the fail is caused by an underlying error (like I/O) the code should just return failure and leave the old error in errno and errmsg? OK then, II adapted the code to cope with this: now it only raises its own errors when appropriate. fail: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + if (!err_msg) +err_msg = DEBUG: mount failed but no error message supplied!; No need to check for consistency in your own code. This might be a good practice in userland programs but here it's a waste of space. Just make sure your code is correct. Ok, removed this particular check (but now there is another one to check whether raising a local error is required). -Habbit -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel ext2_incompat.patch.6 Description: Binary data ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mié, 06-08-2008 a las 12:36 +0200, Felix Zielcke escribió: Am Dienstag, den 05.08.2008, 19:23 +0200 schrieb Felix Zielcke: Am Freitag, den 04.07.2008, 03:20 +0200 schrieb Javier Martín: That was it. I will post no more in this thread. Do whatever you please with the patch - I'll just request some more people from the GRUB dev team to review the thing, instead of the tennis match we've had here (and I appreciate all matches, even the ones I lose). I'd like to bring this topic now up again and yes I know this isn't the last message about it :) Maybe it helps more if I give you a link to the thread start on the archive if you want to read through the whole story again ;) [0] The last mails aboit this was only between Javier and me about which flags should be ignored and which should be marked as supported. Robert was the only one from the official's who commented on the code and from his is even the last message about the topic actually [1]. I really think that it's a good idea. For example currently there exists INCOMPAT_64BIT which only the kernel currently supports but not the e2fsprogs. AFAIK it's probable used for filesystems = max ext3 size, the german Wikipedia ext3 article says 32 TiB the english one 16 TiB Anyway if you use such a real big filesystem in the future even for /boot then in the beginning the /boot stuff is probable at the very beginning of it, but with the time you probable want to make use of it. And then maybe update once the kernel which then probable moves to an area which needs 64bit inode support or whatever this 64bit are used for. Then I think it's better to refuse to install grub to it (e.g. by failing grub-probe) then people leaving in the uncertainness that they may not be able anymore to boot this system. Ok probable nobody ever uses such a big filesystem for their /boot too, but as Javier already said in the thread: There's maybe an ext5, ext6 and so on. By the way: I suggest to rename ext2.mod to extN.mod, on IRC there was already a guy who wondered why it says ext2 instead of ext3 which he had and ext4 extents are now supported which will probable never backported to ext2. [0] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg8.html [1] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00333.html Thanks for raising the topic again. If it serves any purpose, I'll say that the last patch I sent (version 5) is still valid against the current HEAD (rev. 1798) -Habbit ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel 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
Re: grub-probe detects ext4 wronly as ext2
Am Dienstag, den 05.08.2008, 19:23 +0200 schrieb Felix Zielcke: Am Freitag, den 04.07.2008, 03:20 +0200 schrieb Javier Martín: That was it. I will post no more in this thread. Do whatever you please with the patch - I'll just request some more people from the GRUB dev team to review the thing, instead of the tennis match we've had here (and I appreciate all matches, even the ones I lose). I'd like to bring this topic now up again and yes I know this isn't the last message about it :) Maybe it helps more if I give you a link to the thread start on the archive if you want to read through the whole story again ;) [0] The last mails aboit this was only between Javier and me about which flags should be ignored and which should be marked as supported. Robert was the only one from the official's who commented on the code and from his is even the last message about the topic actually [1]. I really think that it's a good idea. For example currently there exists INCOMPAT_64BIT which only the kernel currently supports but not the e2fsprogs. AFAIK it's probable used for filesystems = max ext3 size, the german Wikipedia ext3 article says 32 TiB the english one 16 TiB Anyway if you use such a real big filesystem in the future even for /boot then in the beginning the /boot stuff is probable at the very beginning of it, but with the time you probable want to make use of it. And then maybe update once the kernel which then probable moves to an area which needs 64bit inode support or whatever this 64bit are used for. Then I think it's better to refuse to install grub to it (e.g. by failing grub-probe) then people leaving in the uncertainness that they may not be able anymore to boot this system. Ok probable nobody ever uses such a big filesystem for their /boot too, but as Javier already said in the thread: There's maybe an ext5, ext6 and so on. By the way: I suggest to rename ext2.mod to extN.mod, on IRC there was already a guy who wondered why it says ext2 instead of ext3 which he had and ext4 extents are now supported which will probable never backported to ext2. [0] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg8.html [1] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00333.html ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Am Freitag, den 04.07.2008, 03:20 +0200 schrieb Javier Martín: That was it. I will post no more in this thread. Do whatever you please with the patch - I'll just request some more people from the GRUB dev team to review the thing, instead of the tennis match we've had here (and I appreciate all matches, even the ones I lose). I'd like to bring this topic now up again and yes I know this isn't the last message about it :) My Bugreport on Debian BTS about this is still open just retitele'd [0] I really would like to have ext2.mod reject unknown INCOMPAT flags instead of just failing to boot. It happened to me again that I forgot that I hadn't updated grub2 on a VM where I still prefer ext4 and with playing with grub-legacy I failed to reming that it can't work aynway :) Thanks again to Bean that for me ext4 still works fine, but for the future I'd like to have grub-install failing instead of real grub on the next (re)boot. And thanks again to Robert, telling me in the report to message grub-devel about it, it was the first step joining you :) [0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=488235 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Sat, Jul 05, 2008 at 08:36:13PM +0200, Javier Martín wrote: 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) This overrides the grub_errno and grub_errmsg provided by grub_disk_read and replaces them with values that hide the true problem. If there was a disk read error, we really want to know about it from the lower layer. Well, the old version did just the same (even worse, because the message was generic). What would be the correct path of action here? I mean, how can we propagate the error messages? It shouldn't call grub_error(). fail: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + if (!err_msg) +err_msg = DEBUG: mount failed but no error message supplied!; No need to check for consistency in your own code. This might be a good practice in userland programs but here it's a waste of space. Just make sure your code is correct. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
I see the ext4 patch was checked in recently. Can the forbid-incompat patch with the new, specific error messages be committed too then? I'm submitting an updated version (i.e. against the current HEAD) because new lines were added. PS: does the ext4 patch add support for META_BG? it should be added to the list of supported incompat features then. 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
Re: grub-probe detects ext4 wronly as ext2
From: JavierMartín [EMAIL PROTECTED] Sent: Wednesday, July 16, 2008 5:09 PM To: The development of GRUB 2 grub-devel@gnu.org Subject: Re: grub-probe detects ext4 wronly as ext2 I see the ext4 patch was checked in recently. Can the forbid-incompat patch with the new, specific error messages be committed too then? I'm submitting an updated version (i.e. against the current HEAD) because new lines were added. PS: does the ext4 patch add support for META_BG? it should be added to the list of supported incompat features then. I don't know what this META_BG is but even the ext2 kernel driver supports it [0] Maybe the list of flags have a bit changed so I'm so nice and give you even a git link to the current ext4.h in Linus' official git tree [1] You'll probably mean FLEX_BG :) I didn't take a deep look at the changes between the first patch from Bean and the last one which he commited. But for me it's now working fine with whole / on ext4 made with the final e2fsprogs 1.41 in Debian unstable, with flex_bg,extents and uninit_bg from the INCOMPAT list, so flex_bg and uninit_bg should be added to your list which are ignored/supported Maybe it's just luck for me that it works now with uninit_bg and flex_bg, the best would be if other people would test it [0] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/ext2_fs.h;hb=HEAD [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=fs/ext4/ext4.h;hb=HEAD ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mié, 16-07-2008 a las 17:27 +0200, Felix Zielcke escribió: From: JavierMartín [EMAIL PROTECTED] Sent: Wednesday, July 16, 2008 5:09 PM To: The development of GRUB 2 grub-devel@gnu.org Subject: Re: grub-probe detects ext4 wronly as ext2 I see the ext4 patch was checked in recently. Can the forbid-incompat patch with the new, specific error messages be committed too then? I'm submitting an updated version (i.e. against the current HEAD) because new lines were added. PS: does the ext4 patch add support for META_BG? it should be added to the list of supported incompat features then. I don't know what this META_BG is but even the ext2 kernel driver supports it [0] Er... of course, the Linux extN implementation is the de-facto reference implementation. Some incompat features are only used in newer versions like ext3 and ext4, while others are added to ext2 too. I was talking about the GRUB extN driver, which recently got patched by Bean. Maybe the list of flags have a bit changed so I'm so nice and give you even a git link to the current ext4.h in Linus' official git tree [1] You'll probably mean FLEX_BG :) All those flags make my head spin so fast I'll create a dark hole through gravitomagnetic effects. I no longer know what the hell does each one do T_T I didn't take a deep look at the changes between the first patch from Bean and the last one which he commited. But for me it's now working fine with whole / on ext4 made with the final e2fsprogs 1.41 in Debian unstable, with flex_bg,extents and uninit_bg from the INCOMPAT list, so flex_bg and uninit_bg should be added to your list which are ignored/supported Uninit_bg is signaled (iIrc) in the superblock by a ROCOMPAT flag, GDT_CSUM, and then in the block groups by whatever-it-is (head spinning even faster). Maybe it's just luck for me that it works now with uninit_bg and flex_bg, the best would be if other people would test it AFAIK, uninit_bg should work if the (readonly) GRUB reader respects the spec and skips invalid block groups/inodes/whatever (those that haven't been initialized). As I don't know what the f*** do META_BG and FLEX_BG do, I can't tell you whether they truly work or it's just a matter of luck: it's Bean who can tell us whether or not he implemented support for them - I only added the extents flag to the supported list on my patch, but including more is a matter of seconds. 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
Re: grub-probe detects ext4 wronly as ext2
From: JavierMartín [EMAIL PROTECTED] Sent: Wednesday, July 16, 2008 6:38 PM To: The development of GRUB 2 grub-devel@gnu.org Subject: Re: grub-probe detects ext4 wronly as ext2 Er... of course, the Linux extN implementation is the de-facto reference implementation. Some incompat features are only used in newer versions like ext3 and ext4, while others are added to ext2 too. I was talking about the GRUB extN driver, which recently got patched by Bean. I thought you thought that META_BG is ext4 specific, but even ext2 supports it Anyway I just grepped for META_BG in the e2fsprogs 1.41 source and mke2fs.c says that resize_inode and meta_bg are not compatible resize_inode is by default enabled. I have it disabled on my ext4, because I just don't need it but I don't think this means meta_bg is enabled. So for me it looks like there's no need to worry about meta_bg Maybe I'll find something about that Uninit_bg is signaled (iIrc) in the superblock by a ROCOMPAT flag, GDT_CSUM, and then in the block groups by whatever-it-is (head spinning even faster). Oh well, I gave you a link to ext4.h and I even didn't notice there's really no UNINIT flag in the 3 lists of compat,incompat and rocompat I grepped e2fsprogs source for it You're right it is uninit_bg is the ROCOMPAT flag GDT_CSUM AFAIK, uninit_bg should work if the (readonly) GRUB reader respects the spec and skips invalid block groups/inodes/whatever (those that haven't been initialized). As I don't know what the f*** do META_BG and FLEX_BG do, I can't tell you whether they truly work or it's just a matter of luck: it's Bean who can tell us whether or not he implemented support for them - Yeah, Bean has to tell you. I only looked a few secons at the differences between his 2 patches which made my ext4 working and even if I would have read it longer, I didn't think I would fully understand it :) At [0] there's a mail from me on this list, with 2 quotes from Documentation/ext4.txt and what mke2fs(8) says about it and a quote of Bean what he found about flex_bg I only added the extents flag to the supported list on my patch, but including more is a matter of seconds. Yeah, I just wanted to make sure that uninit_bg and flex_bg don't get forgot, and then I forget this, install the debian package and then end up on a unbootable system :) But even if that happens, it's not a big problem for me now. Currently I only use ext4 in VMware [0] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00157.html ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Oh well I should have used grep with -i meta_bg and META_BG does make a difference Anyway in release-notes I now found this: Add support for the an alternative block group descriptor layout which allows for on-line resizing without needing to prepare the filesystem in advance. (This is the incomat feature flag meta_bg.) So it seems you'll have sooner or later to worry about that. But I still think it's currently not that important to implement it, because it's currently not compatible with resize_inode ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Oh well I should have used grep with -i meta_bg and META_BG does make a difference Anyway in release-notes I now found this: Add support for the an alternative block group descriptor layout which allows for on-line resizing without needing to prepare the filesystem in advance. (This is the incomat feature flag meta_bg.) So it seems you'll have sooner or later to worry about that. But I still think it's currently not that important to implement it, because it's currently not compatible with resize_inode I should really take me more time for such things, but I think it's at least better then sending the same mail twice :) (Robert Millan knows why I say this) Anyway I thought that the release-notes are just for the current release not complete list for every like NEWS or even the changelog This entry is from version 1.30 dated October 31, 2002 So I think resize_inode has completly replaced meta_bg It's at least not mentioned anymore in the mke2fs(8) man page or even set by mke2fs.conf But probably the question remains, if grub should ignore it or reject it if it's set ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mié, 16-07-2008 a las 19:44 +0200, Felix Zielcke escribió: Oh well I should have used grep with -i meta_bg and META_BG does make a difference Anyway in release-notes I now found this: Add support for the an alternative block group descriptor layout which allows for on-line resizing without needing to prepare the filesystem in advance. (This is the incomat feature flag meta_bg.) So it seems you'll have sooner or later to worry about that. But I still think it's currently not that important to implement it, because it's currently not compatible with resize_inode I should really take me more time for such things, but I think it's at least better then sending the same mail twice :) (Robert Millan knows why I say this) Anyway I thought that the release-notes are just for the current release not complete list for every like NEWS or even the changelog This entry is from version 1.30 dated October 31, 2002 So I think resize_inode has completly replaced meta_bg It's at least not mentioned anymore in the mke2fs(8) man page or even set by mke2fs.conf But probably the question remains, if grub should ignore it or reject it if it's set OK, so this is what I get from your 3 posts, and my proposals for the driver future: * meta_bg is a deprecated feature and is incompatible with resize_inode, which is used by default in ext4 and can be used by all other extN filesystems. We cannot safely ignore it since it signals an incompatible change in one of the fs descriptors, so the reading code might not be able to locate (meta)data on disk. I advocate _rejecting_ volumes with it (given that it's no longer being generated by any standard tool) until someone writes a patch to support it. * flex_bg seemingly allows certain metadata structures to be located at places other than their default positions. However, given that it is only used on volumes quite filled with bad blocks, I think we can (at least temporarily) _ignore_ it, but truly supporting it would be a Good Thing (tm) * uninit_bg might already be supported (we should check, though, with bigger partitions) and is not an incompat flag, so my patch does not affect its handling. 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
Re: grub-probe detects ext4 wronly as ext2
From: JavierMartín [EMAIL PROTECTED] Sent: Wednesday, July 16, 2008 9:07 PM To: The development of GRUB 2 grub-devel@gnu.org Subject: Re: grub-probe detects ext4 wronly as ext2 OK, so this is what I get from your 3 posts, and my proposals for the driver future: * meta_bg is a deprecated feature and is incompatible with resize_inode, which is used by default in ext4 and can be used by all other extN filesystems. We cannot safely ignore it since it signals an incompatible change in one of the fs descriptors, so the reading code might not be able to locate (meta)data on disk. I advocate _rejecting_ volumes with it (given that it's no longer being generated by any standard tool) until someone writes a patch to support it. resize_inode is not just enabled for ext4 but for every extN by mke2fs.conf Either check your own /etc/mke2fs.conf or see [0] which is the newest and so has all the ext4 features in it :) I can't remember ever seeing that meta_bg but I do remember that resize_inode wasn't just added now in the newest e2fsprogs The release-notes unfortunatly don't say when it was added or at least I didn't found that with searching for resize_inode * flex_bg seemingly allows certain metadata structures to be located at places other than their default positions. However, given that it is only used on volumes quite filled with bad blocks, I think we can (at least temporarily) _ignore_ it, but truly supporting it would be a Good Thing (tm) For me currently it works but that's why I added that others should test that. At least on this list only I and Bean tested this and I think Bean just tested this with my 30 MB image I gave him, where grub-fstest failed to read the Linux Kernel I don't know what this comment on mke2fs(8) about flex_bg means (use with -G option to group meta-data in order to create a large virtual block group). Oh, I now saw in that mail I forgot to copy the text from that -G option and the raw sourcecode of the manpage I linked there isn't really easy to read -G number-of-groups Specify the number of block goups that will be packed together to create one large virtual block group on an ext4 filesystem. This improves meta-data locality and performance on meta-data heavy workloads. The number of goups must be a power of 2 and may only be specified if the flex_bg filesystem feature is enabled. But because this grouping needs flex_bg enabled, and flex_bg is INCOMPAT, there's maybe a reason grub needs to support it. Maybe I'll try it out if grub fails to boot with that -G option used * uninit_bg might already be supported (we should check, though, with bigger partitions) and is not an incompat flag, so my patch does not affect its handling. I've one 8 GB ext4 where ~ 3 GB are used and another 8 GB ext4 where just ~700 MB are used Both have uninit_bg and flex_bg and grub works with them But because I don't use them much, proberbly grub doestn't even reach the uninitialized blockgroups/inode tables [0] http://git.kernel.org/?p=fs/ext2/e2fsprogs.git;a=blob;f=misc/mke2fs.conf;hb=HEAD ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Fri, Jul 04, 2008 at 10:41:35PM +0200, Javier Martín wrote:
Wonderful! I was sick of jumping through hoops with cvs diff.
I wasn't even using cvs diff! (you don't want to know what my replacement
dance was) ;-)
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
I think that's ok, comments don't eat space, so it's better to risk explaining
too much than being short of explaining everything.
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.
Looks a bit ugly, but I don't have any objection if it makes code smaller
(by eliminating duped grub_error calls).
However, adding new strings is expensive, since they tend to take size more
easily than code. I would be careful about that.
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ const char *err_msg = 0;
Is this const right? You're modifiing its value.
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)
This overrides the grub_errno and grub_errmsg provided by grub_disk_read and
replaces them with values that hide the true problem. If there was a disk
read error, we really want to know about it from the lower layer.
/* 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))
No need to ellaborate here; by definition, the magic number makes the
difference between a corrupt ext2 and something that is not ext2. So
you can just say it's not ext2.
+ /* 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)
Ok.
if (grub_errno)
-goto fail;
+EXT2_DRIVER_MOUNT_FAIL(could not read the root directory inode)
Ok.
--
Robert Millan
GPLv2 I know my rights; I want my phone call!
DRM What good is a phone call… if you are unable to speak?
(as seen on /.)
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El sáb, 05-07-2008 a las 14:07 +0200, Robert Millan escribió:
However, adding new strings is expensive, since they tend to take size more
easily than code. I would be careful about that.
I checked the space requirements, and seemingly there was a bit of space
available in the .rodata zone, since all those strings only added less
than 20 bytes o_O (at least in i386-pc). Wrapping it up, the pre-post
delta including code and strings is 120 bytes in i386-pc.
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ const char *err_msg = 0;
Is this const right? You're modifiing its value.
Yeah, err_msg is a pointer to (const char), thus the characters are
unmodifiable but the pointer can be reassigned. You can also have char
* const, which is a const pointer to char (I don't see much utility
to it); and finally const char * const, a const pointer to (const
char), which would be the constant, unreassignable string pointer.
AFAIK, since GCC 4.0 string literals are const char * by default and
are stored in .rodata, so if the variable was termed as just char *
we'd have needed a cast. However, if the compiler considers string
literals as char *, no cast is needed to make it const char *.
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)
This overrides the grub_errno and grub_errmsg provided by grub_disk_read and
replaces them with values that hide the true problem. If there was a disk
read error, we really want to know about it from the lower layer.
Well, the old version did just the same (even worse, because the message
was generic). What would be the correct path of action here? I mean, how
can we propagate the error messages?
/* 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))
No need to ellaborate here; by definition, the magic number makes the
difference between a corrupt ext2 and something that is not ext2. So
you can just say it's not ext2.
Ok, I'm sending a new version of the thing with that part removed. I'm
trying to think of a ChangeLog entry for this. What about this?
fs/ext2.c: extN driver will reject filesystems with unimplemented
incompatible features enabled in the superblock
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
Re: grub-probe detects ext4 wronly as ext2
Hi, First of all, grub-setup is conservative enough. It read the data twice, one using host os, one using grub, and compare result. If the data is corrupted, the comparison will fail and it will refuse to install. This is an example of good practice, which check the driver by function, not by flags. The flags are a little vague, for example, if they change btree structure in the future, this would introduce another flag, but we don't need to worry about it, because we don't use it at all. If we can ensure the boot partition can be accessed, then the problem is solved. Yes, there may be other partition using ext4 that we can't access, but remember that linux loader will check for signatures as well, a corrupted kernel would not be loaded. So the difference is merely the error message users sees, unknown filesystem or invalid kernel. And, grub WILL follow the evolution the extN, because it's the primary boot loader for linux. The only reason we don't have ext4 support at present is because it's not stable. If major distro starts to use it as default, we would have to support it as well. -- Bean ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
From: Bean [EMAIL PROTECTED]
Sent: Friday, July 04, 2008 8:49 AM
First of all, grub-setup is conservative enough. It read the data
twice, one using host os, one using grub, and compare result. If the
data is corrupted, the comparison will fail and it will refuse to
install.
I thought a few agos as I started to try out ext4 grub-install did success
and then on reboot GRUB just failed with file not found.
I just tried it out now again on Debian unstable with GRUB 2 1.96+20080626-1
with whole / as ext4 with extents,uninit_bg and flex_bg
grub-setup uses 100% CPU and last strace output is this:
open(/dev/sda1, O_RDONLY|O_SYNC) = 3
ioctl(3, BLKFLSBUF, 0) = 0
lseek(3, 523279872, SEEK_SET) = 523279872
read(3, \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0..., 4096) =
4096
close(3)
Attached is the full strace output
For me personally this isn't a problem, I only use ext4 in VMware and I know
now that GRUB doestn't work with it :)
execve(/usr/sbin/grub-setup, [grub-setup, --directory=/boot/grub,
--device-map=/boot/grub/device.m..., /dev/sda], [/* 17 vars */]) = 0
brk(0) = 0x13ac000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded8000
access(/etc/ld.so.nohwcap, F_OK) = -1 ENOENT (No such file or
directory)
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded6000
access(/etc/ld.so.preload, R_OK) = -1 ENOENT (No such file or
directory)
open(/etc/ld.so.cache, O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=7128, ...}) = 0
mmap(NULL, 7128, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7ff16ded4000
close(3)= 0
access(/etc/ld.so.nohwcap, F_OK) = -1 ENOENT (No such file or
directory)
open(/lib/libc.so.6, O_RDONLY)= 3
read(3, \177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0\0\1\0\0\0\300\342..., 832) =
832
fstat(3, {st_mode=S_IFREG|0755, st_size=1379632, ...}) = 0
mmap(NULL, 3486328, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) =
0x7ff16d96a000
mprotect(0x7ff16dab4000, 2097152, PROT_NONE) = 0
mmap(0x7ff16dcb4000, 20480, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x14a000) = 0x7ff16dcb4000
mmap(0x7ff16dcb9000, 17016, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7ff16dcb9000
close(3)= 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded3000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded2000
arch_prctl(ARCH_SET_FS, 0x7ff16ded26e0) = 0
mprotect(0x7ff16dcb4000, 12288, PROT_READ) = 0
munmap(0x7ff16ded4000, 7128)= 0
brk(0) = 0x13ac000
brk(0x13cd000) = 0x13cd000
open(/boot/grub/device.map, O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=30, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded5000
read(3, (hd0)\t/dev/sda\n(hd1)\t/dev/sdb\n, 4096) = 30
stat(/dev/sda, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
lstat(/dev, {st_mode=S_IFDIR|0755, st_size=2580, ...}) = 0
lstat(/dev/sda, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
stat(/dev/sdb, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
lstat(/dev, {st_mode=S_IFDIR|0755, st_size=2580, ...}) = 0
lstat(/dev/sdb, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
read(3, , 4096) = 0
close(3)= 0
munmap(0x7ff16ded5000, 4096)= 0
open(/dev/sda, O_RDONLY) = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed9478) = 0
close(3)= 0
open(/dev/sda, O_RDONLY|O_SYNC) = 3
ioctl(3, BLKFLSBUF, 0) = 0
lseek(3, 8589869056, SEEK_SET) = 8589869056
read(3, \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0..., 4096) =
4096
close(3)= 0
open(/dev/sda, O_RDONLY) = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75eda488) = 0
close(3)= 0
open(/dev/sda, O_RDONLY|O_SYNC) = 3
ioctl(3, BLKFLSBUF, 0) = 0
lseek(3, 0, SEEK_SET) = 0
read(3, \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0..., 512) =
512
read(3, \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0..., 3584) =
3584
close(3)= 0
open(/dev/sda, O_RDONLY) = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed8ff8) = 0
close(3)= 0
stat(/dev/.devfsd, 0x7fff75ed8ed0)= -1 ENOENT (No such file or
directory)
open(/dev/sda1, O_RDONLY) = 3
ioctl(3, 0x301, 0x7fff75ed8f60) = 0
close(3)= 0
open(/dev/sda1,
Re: grub-probe detects ext4 wronly as ext2
I know I promised not to keep posting but... I just cannot xD El vie, 04-07-2008 a las 14:49 +0800, Bean escribió: Hi, This is an example of good practice, which check the driver by function, not by flags. This will find and check core.img. As my previous post showed, one file being readable in a partition with an incompatible flag does not guarantee that every file will be. Thus, the conservative policy for grub-probe would be to also check that the FS is theoretically accesible (i.e. the ext2/whatever driver is fine with its flags) in addition to the current practical check. The flags are a little vague, for example, if they change btree structure in the future, this would introduce another flag, but we don't need to worry about it, because we don't use it at all. The btree flag (and any of its modifications) is ROCOMPAT, which indeed allows us not to worry about it at all, because our driver is read only. The proposed patch addresses only INCOMPAT flags, and it even adds a mechanism to ignore some of them because we deem them not to create real incompatibilities (like needs-recovery). If we can ensure the boot partition can be accessed, then the problem is solved. Yes, there may be other partition using ext4 that we can't access, but remember that linux loader will check for signatures as well, a corrupted kernel would not be loaded. So the difference is merely the error message users sees, unknown filesystem or invalid kernel. I didn't know the linux loader checked signatures. Do the multiboot, multiboot2 and bsd loaders do the same? In the platforms where it is able to load a file, does the chainloader loader perform any checks? So in that cases difference would be between unknown filesystem and the FSM knows what. And, grub WILL follow the evolution the extN, because it's the primary boot loader for linux. The only reason we don't have ext4 support at present is because it's not stable. If major distro starts to use it as default, we would have to support it as well. Please... ReiserFS was used for long in many distros and GRUB2 didn't support it until 1.96 - even with GRUB Legacy having implemented it long ago. I literally waited years for it to be included! Besides, as I already said, we cannot win in a race against the future: new features, some of them incompatible, are introduced constantly (every few years) in extN, and until we get to know about them and at least decide whether they can be safely ignored, the sane behavior is to obey them and reject access (except if the user override is enabled). Yes, we can implement ext4 and possibly even before it's released as stable, but could you please start implementing ext7 so that we don't have to worry about its incompatibilities when it comes? 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
Re: grub-probe detects ext4 wronly as ext2
On Fri, Jul 4, 2008 at 6:34 PM, Javier Martín [EMAIL PROTECTED] wrote: I know I promised not to keep posting but... I just cannot xD El vie, 04-07-2008 a las 14:49 +0800, Bean escribió: Hi, This is an example of good practice, which check the driver by function, not by flags. This will find and check core.img. As my previous post showed, one file being readable in a partition with an incompatible flag does not guarantee that every file will be. Thus, the conservative policy for grub-probe would be to also check that the FS is theoretically accesible (i.e. the ext2/whatever driver is fine with its flags) in addition to the current practical check. Yes, one file can't guarantee the whole fs is accessible, but it's a good indicator. Nothing can guarantee 100% success. Even without structure change, grub can still fail if the underlying fs is dirty. The flags are a little vague, for example, if they change btree structure in the future, this would introduce another flag, but we don't need to worry about it, because we don't use it at all. The btree flag (and any of its modifications) is ROCOMPAT, which indeed allows us not to worry about it at all, because our driver is read only. The proposed patch addresses only INCOMPAT flags, and it even adds a mechanism to ignore some of them because we deem them not to create real incompatibilities (like needs-recovery). It's just an example. We only use part of ext's structure, there is every chance that some part of it has been changed and we're not affected. If we can ensure the boot partition can be accessed, then the problem is solved. Yes, there may be other partition using ext4 that we can't access, but remember that linux loader will check for signatures as well, a corrupted kernel would not be loaded. So the difference is merely the error message users sees, unknown filesystem or invalid kernel. I didn't know the linux loader checked signatures. Do the multiboot, multiboot2 and bsd loaders do the same? In the platforms where it is able to load a file, does the chainloader loader perform any checks? So in that cases difference would be between unknown filesystem and the FSM knows what. Yes, they all check for signatures. In fact, commands that deal with input file should always check for it first. If not, we need to fix that command. And, grub WILL follow the evolution the extN, because it's the primary boot loader for linux. The only reason we don't have ext4 support at present is because it's not stable. If major distro starts to use it as default, we would have to support it as well. Please... ReiserFS was used for long in many distros and GRUB2 didn't support it until 1.96 - even with GRUB Legacy having implemented it long ago. I literally waited years for it to be included! Besides, as I already said, we cannot win in a race against the future: new features, some of them incompatible, are introduced constantly (every few years) in extN, and until we get to know about them and at least decide whether they can be safely ignored, the sane behavior is to obey them and reject access (except if the user override is enabled). Yes, we can implement ext4 and possibly even before it's released as stable, but could you please start implementing ext7 so that we don't have to worry about its incompatibilities when it comes? It's an ongoing process for grub. Besides, compatibility goes both ways. The developer of extN file system would also consider existing driver, significant change to the fs structure would not be as frequent. -- Bean ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El vie, 04-07-2008 a las 19:29 +0800, Bean escribió: On Fri, Jul 4, 2008 at 6:34 PM, Javier Martín [EMAIL PROTECTED] wrote: I know I promised not to keep posting but... I just cannot xD El vie, 04-07-2008 a las 14:49 +0800, Bean escribió: Hi, This is an example of good practice, which check the driver by function, not by flags. This will find and check core.img. As my previous post showed, one file being readable in a partition with an incompatible flag does not guarantee that every file will be. Thus, the conservative policy for grub-probe would be to also check that the FS is theoretically accesible (i.e. the ext2/whatever driver is fine with its flags) in addition to the current practical check. Yes, one file can't guarantee the whole fs is accessible, but it's a good indicator. Nothing can guarantee 100% success. Even without structure change, grub can still fail if the underlying fs is dirty. The flags are a little vague, for example, if they change btree structure in the future, this would introduce another flag, but we don't need to worry about it, because we don't use it at all. The btree flag (and any of its modifications) is ROCOMPAT, which indeed allows us not to worry about it at all, because our driver is read only. The proposed patch addresses only INCOMPAT flags, and it even adds a mechanism to ignore some of them because we deem them not to create real incompatibilities (like needs-recovery). It's just an example. We only use part of ext's structure, there is every chance that some part of it has been changed and we're not affected. That's the point of categorizing new features in rw-compat, ro-compat and incompat. The first two indicate changes that won't affect any reader, while the latter warn readers that they will very probably find incompatible metadata which they will not be able to interpret. From the _current_ list of incompatible features: compression, filetype, needs_recovery, journal_dev, meta_bg, extents, 64bit and flex_bg; we have implemented support for one (filetype) and knowingly ignore another (needs_recovery). However, every other incompat flag affects the metadata format in ways that cause strange failures difficult for the user to trace, like the Godfather tune in my other post being read as 256 bytes of zeros. As an example, one of the _bg flags (meta_bg iIrc) means that inodes may be lazily written to disk instead of every single one being initialized by mke2fs, which speeds up the format process a lot. Given that our driver does not know what to do with that incompat feature (and it currently ignores it), if our driver met one of the inodes not yet inited, we would read either garbage or even worse, metadata from another extN filesystem that was there before the last mke2fs. If we can ensure the boot partition can be accessed, then the problem is solved. Yes, there may be other partition using ext4 that we can't access, but remember that linux loader will check for signatures as well, a corrupted kernel would not be loaded. So the difference is merely the error message users sees, unknown filesystem or invalid kernel. I didn't know the linux loader checked signatures. Do the multiboot, multiboot2 and bsd loaders do the same? In the platforms where it is able to load a file, does the chainloader loader perform any checks? So in that cases difference would be between unknown filesystem and the FSM knows what. Yes, they all check for signatures. In fact, commands that deal with input file should always check for it first. If not, we need to fix that command. They all check for _certain_ signatures in particular places, like multiboot looking for the MB-info block and chainloader looking for 0x55aa (in i386-pc), but I already explained how a hypothetical new incompatible feature like partial compression can make that system go down the sink, because the block with the signature can well be fine but other parts be read wrong. And, grub WILL follow the evolution the extN, because it's the primary boot loader for linux. The only reason we don't have ext4 support at present is because it's not stable. If major distro starts to use it as default, we would have to support it as well. Please... ReiserFS was used for long in many distros and GRUB2 didn't support it until 1.96 - even with GRUB Legacy having implemented it long ago. I literally waited years for it to be included! Besides, as I already said, we cannot win in a race against the future: new features, some of them incompatible, are introduced constantly (every few years) in extN, and until we get to know about them and at least decide whether they can be safely ignored, the sane behavior is to obey them and reject access (except if the user override is enabled). Yes, we can implement ext4 and possibly even before it's released as stable, but could you please start implementing ext7 so that
Re: grub-probe detects ext4 wronly as ext2
On Fri, Jul 04, 2008 at 12:34:50PM +0200, Javier Martín wrote: And, grub WILL follow the evolution the extN, because it's the primary boot loader for linux. The only reason we don't have ext4 support at present is because it's not stable. If major distro starts to use it as default, we would have to support it as well. Please... ReiserFS was used for long in many distros and GRUB2 didn't support it until 1.96 - even with GRUB Legacy having implemented it long ago. I literally waited years for it to be included! This is not a good example. We were (and still are, though almost finished) in the process of transitioning from GRUB Legacy to GRUB 2. ReiserFS authors weren't compelled to contribute a driver for GRUB 2 the same way they had been to contribute it for GRUB Legacy. Besides, as I already said, we cannot win in a race against the future: new features, some of them incompatible, are introduced constantly (every few years) in extN, and until we get to know about them and at least decide whether they can be safely ignored, the sane behavior is to obey them and reject access (except if the user override is enabled). Yes, we can implement ext4 and possibly even before it's released as stable, but could you please start implementing ext7 so that we don't have to worry about its incompatibilities when it comes? If this is really to be considered a problem, we might as well be conservative in both grub-probe and real GRUB, and reject unknown flags. Not a big deal, since unknown flags aren't really going to use their experimental status untill mainstream distributions support them, and this includes GRUB. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Fri, Jul 04, 2008 at 02:00:34PM +0200, Javier Martín wrote: In the nearly eight years from ext2 release to the merge of ext3 into the kernel, some incompatible features were introduced in ext2 proper, like filetype and compression. Even now, with ext4 in development and merged into the kernel less than five years after ext3, incompatible features are being backported to ext2 like the lazy inode initializing I mentioned earlier. Those can hit us HARD. So you're saying we should make our decisions based on the assumption that Linux developers could be jeopardizing our efforts by introducing incompatible features by surprise, without any coordination with us, and that distributors are going to pass those features down to stable releases, knowing that they break GRUB, and again without any coordination with us? -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
If you wish to send a patch for the user environment check you proposed, could send it separately? I'm inclined to commit the flag check, but not the environment stuff. Afterwards if you like we could discuss about whether we should be best-effort in real GRUB or not. It's really not such a big deal, as long as we don't end up cluttering the user interface because of it. On Fri, Jul 04, 2008 at 03:32:43AM +0200, Javier Martín wrote: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem, or incompatible + features enabled (extents, etc.)); I would avoid referring explicitly to the features involved, because it means the message will end up being obsolete when extents are supported (and we will likely forget to update it). -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Fri, Jul 04, 2008 at 04:04:56PM +0200, Robert Millan wrote: If this is really to be considered a problem, we might as well be conservative in both grub-probe and real GRUB, and reject unknown flags. Not a big deal, since unknown flags aren't really going to use their experimental status untill mainstream distributions support them, and this includes GRUB. I meant to lose rather than to use -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El vie, 04-07-2008 a las 16:09 +0200, Robert Millan escribió: On Fri, Jul 04, 2008 at 02:00:34PM +0200, Javier Martín wrote: In the nearly eight years from ext2 release to the merge of ext3 into the kernel, some incompatible features were introduced in ext2 proper, like filetype and compression. Even now, with ext4 in development and merged into the kernel less than five years after ext3, incompatible features are being backported to ext2 like the lazy inode initializing I mentioned earlier. Those can hit us HARD. So you're saying we should make our decisions based on the assumption that Linux developers could be jeopardizing our efforts by introducing incompatible features by surprise, without any coordination with us, and that distributors are going to pass those features down to stable releases, knowing that they break GRUB, and again without any coordination with us? I'm not saying that they will _consciously_ try to break us, just that new flags can be introduced just like that and that, while ext4 is definitely making quite a fuss, the real danger is in flags like EXT2_FEATURE_INCOMPAT_META_BG, the lazy inode writing flag I mentioned to Bean, since a best-effort reader that ignores it can (with a very high probability) read old metadata, since if the same partition is re-formatted with ext2 the inodes will be on the same places. Thus, files from the old FS could resurrect (with their data in unknown state) and other nasty things. These kind of flags don't make a lot of noise in developer circles outside LKML, they are introduced without long times in development state and then our driver becomes broken in a way that will be very difficult to bugtrace. That's why I said that the bootloader should adopt a conservative stance WRT incompatible feature flags too, but the user override should be present in order to give informed users a choice without having to rebuild GRUB from source. 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
Re: grub-probe detects ext4 wronly as ext2
El vie, 04-07-2008 a las 22:11 +0800, Bean escribió: About the flags, no one can guarantee that the incompat bit would result in an unreadable fs. The journal is a good example. Even with journal enabled, we can read from it most of the time. The journal: EXT3_FEATURE_COMPAT_HAS_JOURNAL (R/W compatibility) The btree: EXT2_FEATURE_RO_COMPAT_BTREE_DIR (R/O compatibility) Can you grasp the difference between these back-compatible additions to the filesystem and incompatible changes that break readers like the EXT2_FEATURE_INCOMPAT_META_BG lazy inode writing I mentioner earlier? Note that the /boot directory is rarely touched, and we sync twice when modifying it. You may argue that there is still a chance of failure. Well, there certainly is, but should we disable install to ext3 ? No, because the journal flag has read-write compatibility: we are guaranteed to be able to mess with the FS with our current driver. 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
Re: grub-probe detects ext4 wronly as ext2
El vie, 04-07-2008 a las 16:21 +0200, Robert Millan escribió: If you wish to send a patch for the user environment check you proposed, could send it separately? I'm inclined to commit the flag check, but not the environment stuff. Ok, here is the old version of the patch without the user override thingy (and the extents reference removed). Afterwards if you like we could discuss about whether we should be best-effort in real GRUB or not. It's really not such a big deal, as long as we don't end up cluttering the user interface because of it. By the way, it's true that UI should be carefully thought of and we should exercise great care not to end like GRUB Legacy with its quirks and such, but... this is a bootloader, not Mac OS X! Even though it should have simple paths, it's supposed to be complex (within reason) On Fri, Jul 04, 2008 at 03:32:43AM +0200, Javier Martín wrote: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem, or incompatible + features enabled (extents, etc.)); I would avoid referring explicitly to the features involved, because it means the message will end up being obsolete when extents are supported (and we will likely forget to update it). True - parenthesized part removed form the patch. By the way, I'm already using SVN (and thus svn diff) for this patch. Is that right? Was the migration completed already? Index: fs/ext2.c === --- fs/ext2.c (revisión: 1687) +++ fs/ext2.c (copia de trabajo) @@ -71,8 +71,39 @@ ? 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) */ +#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) */ +#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) */ +#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 /* Journal device */ +#define EXT2_FEATURE_INCOMPAT_META_BG 0x0010 +#define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 /* extents support */ +#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 ) +/* 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 ) + + #define EXT3_JOURNAL_MAGIC_NUMBER 0xc03b3998U #define EXT3_JOURNAL_DESCRIPTOR_BLOCK 1 @@ -394,6 +425,12 @@ if (grub_le_to_cpu16 (data-sblock.magic) != EXT2_MAGIC) goto fail; + /* 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)) +goto fail; + + data-disk = disk; data-diropen.data = data; @@ -409,7 +446,8 @@ return data; fail: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem, or incompatible + features enabled); 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
Re: grub-probe detects ext4 wronly as ext2
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. +/* Superblock filesystem feature flags (RW compatible) */ +#define EXT2_FEATURE_COMPAT_DIR_PREALLOC 0x0001 +#define EXT2_FEATURE_COMPAT_IMAGIC_INODES0x0002 +#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_INDEX0x0020 +/* Superblock filesystem feature flags (RO compatible) */ +#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001 +#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE0x0002 +#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) */ +#define EXT2_FEATURE_INCOMPAT_COMPRESSION0x0001 +#define EXT2_FEATURE_INCOMPAT_FILETYPE 0x0002 +#define EXT3_FEATURE_INCOMPAT_RECOVER0x0004 /* Needs recovery */ +#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV0x0008 /* Journal device */ +#define EXT2_FEATURE_INCOMPAT_META_BG0x0010 +#define EXT4_FEATURE_INCOMPAT_EXTENTS0x0040 /* extents support */ +#define EXT4_FEATURE_INCOMPAT_64BIT 0x0080 +#define EXT4_FEATURE_INCOMPAT_FLEX_BG0x0200 I'd suggest making the RW compatible etc notes a bit more ellaborate to make it clear what they mean (I'm confused myself). +/* 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). +/* 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. Maybe Pavel can ellaborate more, I think it was him who brought up this point. + /* 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)) +goto fail; [...] fail: - grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem); + grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem, or incompatible + features enabled); 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. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
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
Re: grub-probe detects ext4 wronly as ext2
On Wed, Jul 02, 2008 at 09:32:40PM +0200, Javier Martín wrote: El mié, 02-07-2008 a las 16:22 +0200, Robert Millan escribió: On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote: A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. We don't have any problem to pass to users: ext4 is not supported We don't have an urge to support ext4, but that doesn't mean we shouldn't consider it a problem. I think adding an interface for the user to choose in which way to deal with our limitations is a nasty thing. I strongly object to that. May I ask why? Is it thus better to impose our own way of dealing with our limitations, unchangeable and possibly wrong in some instances (see below for a possible scenario)? Sensible defaults are good, but choice is one of the bases of freedom. We're never in a position in which we can impose things, because GRUB is free software to begin with, and anyone can modify it to better suit their needs. The question here is whether we should increase complexity with interfaces that don't provide any usefulness to the user, just to make it easier to do potentially dangerous things. If a user understands the implications and really doesn't mind making her system bootability unreliable, then she should have no problem modifiing the source to do it. The problem with INCOMPAT_* flags is that we cannot always know what they mean, and thus, a best-effort reader risks not just bumping into metadata it does not understand (and thus crashing or spitting thousands of errors if it's not robust enough), This should never happen; see the remark about corrupt filesystems in my previous mail. but also ignoring it and reading wrong data to memory. This looks like a more general problem. I wonder if we should really address it at the filesystem level. e.g. the filesystem could be perfectly fine but the files in it went corrupt; if the data you're reading is corrupt, does it matter at which point it was corrupted? A more elegant solution (also may be interesting for security at some point) would be for update-grub to hash each file it generates access commands for and embed the sum in grub.cfg as a check parameter, like if verify_hash /file x ; then do_something_with_file /file fi So, if we take for granted those two things: - That GRUB should never crash no matter what you feed to it. - That update-grub instructs GRUB to verify file consistency via hashing. does this address all of your concerns? -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Robert Millan wrote: A more elegant solution (also may be interesting for security at some point) would be for update-grub to hash each file it generates access commands for and embed the sum in grub.cfg as a check parameter, like if verify_hash /file x ; then do_something_with_file /file fi So, if we take for granted those two things: - That GRUB should never crash no matter what you feed to it. - That update-grub instructs GRUB to verify file consistency via hashing. also?, - That whenever someone wants to boot a new kernel (or whatever), they re-run update-grub. Which definitely doesn't apply if they're interactively poking around with the GRUB commandline. But it could be a safety check for some cases. Would it ever make sense to *ask* the user whether to proceed, if the file is different? (they might have changed the file deliberately!) But, with that code you mentioned for grub.cfg, I suppose it can be adjusted to do that, if desired by whoever controls grub.cfg. -Isaac ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El jue, 03-07-2008 a las 16:02 +0200, Robert Millan escribió: On Wed, Jul 02, 2008 at 09:32:40PM +0200, Javier Martín wrote: El mié, 02-07-2008 a las 16:22 +0200, Robert Millan escribió: On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote: A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. We don't have any problem to pass to users: ext4 is not supported We don't have an urge to support ext4, but that doesn't mean we shouldn't consider it a problem. I think adding an interface for the user to choose in which way to deal with our limitations is a nasty thing. I strongly object to that. May I ask why? Is it thus better to impose our own way of dealing with our limitations, unchangeable and possibly wrong in some instances (see below for a possible scenario)? Sensible defaults are good, but choice is one of the bases of freedom. We're never in a position in which we can impose things, because GRUB is free software to begin with, and anyone can modify it to better suit their needs. GRUB is bundled with many Linux distros, and while it can be substituted for others, being the default choice should entail a bit of responsibility. Please, don't be like M$ with their oh, if the users don't like us, they can always switch (after overriding lock-in). The question here is whether we should increase complexity with interfaces that don't provide any usefulness to the user, just to make it easier to do potentially dangerous things. If a user understands the implications and really doesn't mind making her system bootability unreliable, then she should have no problem modifiing the source to do it. The system bootability is already unreliable with the current code, and as I already explained, it will be unreliable as long as the filesystem drivers use the best-effort politics you support. I'm just proposing that we change the default politics in the bootloader to nearly conservative and then having an user-controllable option to enable best-effort behaviour. I've had GRUB since at least 2004, and I've done a few nasty things in its command line from the start; but only now I feel comfortable enough to modify its source, so don't assume that a knowledgeable/advanced user will be brave enough to modify the source of his _bootloader_ just like that. I don't understand why you're so adamant against sensible defaults AND the possibility of a rational, free choice. but also ignoring it and reading wrong data to memory. This looks like a more general problem. I wonder if we should really address it at the filesystem level. e.g. the filesystem could be perfectly fine but the files in it went corrupt; if the data you're reading is corrupt, does it matter at which point it was corrupted? It does, if the data on disk is not corrupted and our filesystem driver reads wrong data because in its best-effort trials to read the data it forfeits the specification-mandated behaviour of aborting on finding incompatible feature flags. We would be INTRODUCING errors in the data, instead of just reading erroneous data because of a crash or something like that. A more elegant solution (also may be interesting for security at some point) would be for update-grub to hash each file it generates access commands for and embed the sum in grub.cfg as a check parameter, like if verify_hash /file x ; then do_something_with_file /file fi This is fine for update-grub, but the GRUB2 scripting language is complex enough that detecting every file access without missing any can get quite complex, and we'd need to embed even more code in the kernel (the hash verifier). I've implemented MD5 and SHA1 in various programming languages as a simple challenge and I can tell you that, while not the toughest thing in the world, it's not simple to get right and it means even less space in an already big core.img. So, if we take for granted those two things: - That GRUB should never crash no matter what you feed to it. - That update-grub instructs GRUB to verify file consistency via hashing. does this address all of your concerns? It would, on a perfect world, but making all the FS driver routines tough enough to provably ensure that they will never crash no matter what is fed into them will probably enlarge the code size too much. WRT the proposed hashing, it does not take into account the use of the GRUB command line and, as I already mentioned, can also fail. I'm finding this discussion increasingly unproductive because it's drifting over an ideological issue (whether or not switch the reading policy to more conservative and give users an override over it), so I will not push the issue much further. I'll probably send in a new version of the patch with the user override option implemented within a few days and let the devs decide about it.
Re: grub-probe detects ext4 wronly as ext2
On Thu, Jul 03, 2008 at 07:07:55PM +0200, Javier Martín wrote: The question here is whether we should increase complexity with interfaces that don't provide any usefulness to the user, just to make it easier to do potentially dangerous things. If a user understands the implications and really doesn't mind making her system bootability unreliable, then she should have no problem modifiing the source to do it. The system bootability is already unreliable with the current code, You mean because grub-probe is not conservative enough when determining whether a filesystem can be accessed? I think we agreed that this needs fixing. and as I already explained, it will be unreliable as long as the filesystem drivers use the best-effort politics you support. If grub-probe says you can't rely on this filesystem being readable by GRUB, then we're bound to that. You don't make it any more reliable by adding user input into the equation. However, you can avoid the problem by refusing to access that filesystem (perhaps by disabling font loading, or whatever we needed from it). It's at that point you can rely on GRUB being able to boot. When I talk about real GRUB being best-effort, it's not reliability that is at stake here; we already got that from grub-probe. This is a minor problem IMHO; it's just about having read access to as many filesystems as possible (other than the one we need for booting) vs not risking corruption during data read. I seriously think we're wasting our time here. Really, it's not worth it. This whole discussion is not about how we engineer a new feature or solve a bug, but about how we deal with a _temporary_ limitation in our code. I'm just proposing that we change the default politics in the bootloader to nearly conservative and then having an user-controllable option to enable best-effort behaviour. I've had GRUB since at least 2004, and I've done a few nasty things in its command line from the start; but only now I feel comfortable enough to modify its source, so don't assume that a knowledgeable/advanced user will be brave enough to modify the source of his _bootloader_ just like that. I don't understand why you're so adamant against sensible defaults AND the possibility of a rational, free choice. Because it's a gratuitous increase in complexity of the user interface. It's a choice about a bug, which is due to be fixed, and for which an informed answer will always be the same. This looks like a more general problem. I wonder if we should really address it at the filesystem level. e.g. the filesystem could be perfectly fine but the files in it went corrupt; if the data you're reading is corrupt, does it matter at which point it was corrupted? It does, if the data on disk is not corrupted and our filesystem driver reads wrong data because in its best-effort trials to read the data it forfeits the specification-mandated behaviour of aborting on finding incompatible feature flags. We would be INTRODUCING errors in the data, instead of just reading erroneous data because of a crash or something like that. When the problem is you've read corrupt data, and you have to handle this gracefuly, it doesn't make any difference that it was your fault because you violated a spec, it's still the same problem. This is fine for update-grub, but the GRUB2 scripting language is complex enough that detecting every file access without missing any can get quite complex, and we'd need to embed even more code in the kernel (the hash verifier). I've implemented MD5 and SHA1 in various programming languages as a simple challenge and I can tell you that, while not the toughest thing in the world, it's not simple to get right and it means even less space in an already big core.img. Why in the kernel? It's essential that during install time you can rely on /boot/grub being readable (otherwise I don't think we should support install at all). I'm finding this discussion increasingly unproductive Hey, at least we agree on something :-) -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El vie, 04-07-2008 a las 02:08 +0200, Robert Millan escribió: On Thu, Jul 03, 2008 at 07:07:55PM +0200, Javier Martín wrote: The question here is whether we should increase complexity with interfaces that don't provide any usefulness to the user, just to make it easier to do potentially dangerous things. If a user understands the implications and really doesn't mind making her system bootability unreliable, then she should have no problem modifiing the source to do it. The system bootability is already unreliable with the current code, You mean because grub-probe is not conservative enough when determining whether a filesystem can be accessed? I think we agreed that this needs fixing. Indeed, grub-probe should adopt the most conservative stance possible so that real GRUB works on minimum assurances. However, I meant reliability was compromised because grub-probe, as currently used by grub-install on i386-pc, only checks the availability of the filesystem GRUB will boot from. Thus, if GRUB is installed on a normal ext2/fat/whatever partition and the OS kernel we will boot (or another file we want to use) is stored in an ext4 partition, the most likely output of our current scheme is an error from the ext2 driver (ranging from an error message to a crash), because in its current best-effort policy it ignored the forbidden signs in the superblock and tried to read the ext4 partition as ext2. and as I already explained, it will be unreliable as long as the filesystem drivers use the best-effort politics you support. If grub-probe says you can't rely on this filesystem being readable by GRUB, then we're bound to that. You don't make it any more reliable by adding user input into the equation. As I said, there are setups in which (the current use of) grub-probe does not add any reliability, because it just checks the GRUB boot partition, not those with the kernels we might load (and I'm not proposing instituting such checks). When I talk about real GRUB being best-effort, it's not reliability that is at stake here; we already got that from grub-probe. This is a minor problem IMHO; it's just about having read access to as many filesystems as possible (other than the one we need for booting) vs not risking corruption during data read. I seriously think we're wasting our time here. Really, it's not worth it. This whole discussion is not about how we engineer a new feature or solve a bug, but about how we deal with a _temporary_ limitation in our code. It is both a temporary limitation and a permanent one: the limitation of not being able to read ext4 extents and such is temporary indeed, but I already said why we cannot just ignore the incompat field. Today is ext4, tomorrow it will be something else. As I mentioned above, such flags are huge warning signs telling us not to dare enter the partition if we don't know what to do with them. I'm just proposing that we change the default politics in the bootloader to nearly conservative and then having an user-controllable option to enable best-effort behaviour. I've had GRUB since at least 2004, and I've done a few nasty things in its command line from the start; but only now I feel comfortable enough to modify its source, so don't assume that a knowledgeable/advanced user will be brave enough to modify the source of his _bootloader_ just like that. I don't understand why you're so adamant against sensible defaults AND the possibility of a rational, free choice. Because it's a gratuitous increase in complexity of the user interface. It's a choice about a bug, which is due to be fixed, and for which an informed answer will always be the same. As I said before, we can fix the ext4 compatibility not implemented bug, we WILL eventually encounter _another_ incompatible feature, and then another... We can't be in par with the evolution of extN filesystems! This looks like a more general problem. I wonder if we should really address it at the filesystem level. e.g. the filesystem could be perfectly fine but the files in it went corrupt; if the data you're reading is corrupt, does it matter at which point it was corrupted? It does, if the data on disk is not corrupted and our filesystem driver reads wrong data because in its best-effort trials to read the data it forfeits the specification-mandated behaviour of aborting on finding incompatible feature flags. We would be INTRODUCING errors in the data, instead of just reading erroneous data because of a crash or something like that. When the problem is you've read corrupt data, and you have to handle this gracefuly, it doesn't make any difference that it was your fault because you violated a spec, it's still the same problem. Fine, then. Let's _not_ worry about automatically trying to read what we cannot, violating standards and such. After all, GRUB _is_ better than Internet Explorer. It _does_
Re: grub-probe detects ext4 wronly as ext2
And, after writing all that, I forgot to append the patch. Sigh.
Index: fs/ext2.c
===
--- fs/ext2.c (revisión: 1687)
+++ fs/ext2.c (copia de trabajo)
@@ -50,6 +50,7 @@
#include grub/dl.h
#include grub/types.h
#include grub/fshelp.h
+#include grub/env.h
/* Log2 size of ext2 block in 512 blocks. */
#define LOG2_EXT2_BLOCK_SIZE(data) \
@@ -71,8 +72,42 @@
? 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) */
+#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) */
+#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) */
+#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 /* Journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG 0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 /* extents support */
+#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 )
+/* 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 )
+
+/* Strings for the driver environment options */
+#define EXT2_DRIVER_ENVOPT_VAR ext2_options
+#define EXT2_DRIVER_ENVOPT_IGNOREINCOMPAT ignore_incompatible
+
#define EXT3_JOURNAL_MAGIC_NUMBER 0xc03b3998U
#define EXT3_JOURNAL_DESCRIPTOR_BLOCK 1
@@ -379,6 +414,7 @@
grub_ext2_mount (grub_disk_t disk)
{
struct grub_ext2_data *data;
+ char* env_options = grub_env_get (EXT2_DRIVER_ENVOPT_VAR);
data = grub_malloc (sizeof (struct grub_ext2_data));
if (!data)
@@ -394,6 +430,14 @@
if (grub_le_to_cpu16 (data-sblock.magic) != EXT2_MAGIC)
goto fail;
+ /* Check the FS doesn't have feature bits enabled that we don't support.
+ * Ignore this check if the ignore_incompatible env. option is set */
+ if (0 != (grub_le_to_cpu32 (data-sblock.feature_incompat)
+ ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+ (0 == grub_strstr (env_options, EXT2_DRIVER_ENVOPT_IGNOREINCOMPAT)))
+goto fail;
+
+
data-disk = disk;
data-diropen.data = data;
@@ -409,7 +453,8 @@
return data;
fail:
- grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem);
+ grub_error (GRUB_ERR_BAD_FS, not an ext2 filesystem, or incompatible
+ features enabled (extents, etc.));
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
Re: grub-probe detects ext4 wronly as ext2
On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote: A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. We don't have any problem to pass to users: ext4 is not supported We don't have an urge to support ext4, but that doesn't mean we shouldn't consider it a problem. I think adding an interface for the user to choose in which way to deal with our limitations is a nasty thing. I strongly object to that. and thus we do the Right Thing (tm) in patching our ext2 driver so that it won't try to read a filesystem it cannot. That makes sense, with some caveats (see below). However, given Pavel's and others' objections, I suggested the addition of an user override to it. Thus, the user will have to knowingly force the system to interpret the filesystem with its current code, and accept any failures he might get, instead of the current behaviour of having the FS mounted automatically without checking incompatibilities (and then getting the errors anyway). I don't think this is necessary. First, let's take for granted that our code is in every situation smart enough not to crash when a filesystem isn't readable (this should always be the case, since we might occasionaly be asked to read corrupt filesystems). Then, what do flags mean? If a flag means GRUB won't be able to access this filesystem at all, we could explicitly refuse to probe it, but then again our code must be graceful enough to cope with it without crashing anyway (see above), so maybe it's not worth to (depends on the time/size trade-off). If a flag means access to the filesystem isn't deterministic, and grub-probe might be able to do things that real GRUB won't, then we're in a situation in which we'd like grub-probe to be conservative _but_ real GRUB to be best-effort. I think this means an internal switch to tell fs probes whether to be conservative or not. We could even use #ifdef GRUB_UTIL so the flag checking stuff doesn't make real GRUB fatter. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Wed, 2008-07-02 at 16:22 +0200, Robert Millan wrote: I don't think this is necessary. First, let's take for granted that our code is in every situation smart enough not to crash when a filesystem isn't readable (this should always be the case, since we might occasionaly be asked to read corrupt filesystems). Good point. If a flag means access to the filesystem isn't deterministic, and grub-probe might be able to do things that real GRUB won't, then we're in a situation in which we'd like grub-probe to be conservative _but_ real GRUB to be best-effort. I think this means an internal switch to tell fs probes whether to be conservative or not. We could even use #ifdef GRUB_UTIL so the flag checking stuff doesn't make real GRUB fatter. Another good point. We should not let users install GRUB on a filesystem that may eventually become inaccessible. Still, we probably want grub-fstest and grub-emu to be best effort to be able to debug compatibility problems. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mié, 02-07-2008 a las 16:22 +0200, Robert Millan escribió: On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote: A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. We don't have any problem to pass to users: ext4 is not supported We don't have an urge to support ext4, but that doesn't mean we shouldn't consider it a problem. I think adding an interface for the user to choose in which way to deal with our limitations is a nasty thing. I strongly object to that. May I ask why? Is it thus better to impose our own way of dealing with our limitations, unchangeable and possibly wrong in some instances (see below for a possible scenario)? Sensible defaults are good, but choice is one of the bases of freedom. However, given Pavel's and others' objections, I suggested the addition of an user override to it. Thus, the user will have to knowingly force the system to interpret the filesystem with its current code, and accept any failures he might get, instead of the current behaviour of having the FS mounted automatically without checking incompatibilities (and then getting the errors anyway). I don't think this is necessary. First, let's take for granted that our code is in every situation smart enough not to crash when a filesystem isn't readable (this should always be the case, since we might occasionaly be asked to read corrupt filesystems). Then, what do flags mean? If a flag means GRUB won't be able to access this filesystem at all, we could explicitly refuse to probe it, but then again our code must be graceful enough to cope with it without crashing anyway (see above), so maybe it's not worth to (depends on the time/size trade-off). If a flag means access to the filesystem isn't deterministic, and grub-probe might be able to do things that real GRUB won't, then we're in a situation in which we'd like grub-probe to be conservative _but_ real GRUB to be best-effort. I think this means an internal switch to tell fs probes whether to be conservative or not. We could even use #ifdef GRUB_UTIL so the flag checking stuff doesn't make real GRUB fatter. The problem with INCOMPAT_* flags is that we cannot always know what they mean, and thus, a best-effort reader risks not just bumping into metadata it does not understand (and thus crashing or spitting thousands of errors if it's not robust enough), but also ignoring it and reading wrong data to memory. In some circumstances, this can lead to nasty bugs, and this is the reason I want the override-incompatible-flags loophole in the driver to require explicit user activation. We can decide what to do with flags we currently know, like ext4 flags (extents and such), and that's the purpose of the IGNORE_INCOMPAT macro in the patch; but with future flags we have no clue. Consider a hypothetical ext5 file system with a new INCOMPAT_BLOCKCOMP feature flag set; and without any other unimplemented flags like extents and such. This new flag will mean that _some_ blocks of a file are LZMA-compressed and some are not (maybe to the criterion of the writing driver, like more than 5% space savings or such). The info on which blocks are compressed and which are not is saved on a bitmap linked to from an extended attribute in the inode (I'm making this out right now, so this might be impossible as described). If our current driver found this filesystem and tried to read a multiboot kernel from it, it might read the whole file as it if were uncompressed, thus putting a corrupt image into memory, possibly even reading past the end of the file in the disk, or less data than the stated file size. If the first blocks (with the multiboot headers) were among the uncompressed ones, GRUB would happily boot from it, thus leaving it to the criterion of the booted kernel what to do. The end result might be just a triple fault when the CPU runs into compressed code; or may come to the kernel doing something nasty to the computer due to a corrupt HD driver commands structure, for example. This scenario would be completely transparent to the user because GRUB would mount the FS without even warning the user. Thus, I think that best-effort is not always preferable when we're dealing with unknown INCOMPAT_* flags. Those flags mean, by default, you can't read this filesystem if you don't know how to interpret this. Thus, I think GRUB should initially reject to mount any such filesystem, but provide an override for two cases: 1) The user explicitly requests it. Maybe he's trying to boot an older kernel which was not compressed in a filesystem that just got accidentally converted, or just check the source of the error. 2) GRUB is trying to boot from such a partition This can be just as risky as the scenario I depicted before, but in this case we might really not have another way out, since the user override would be
Re: grub-probe detects ext4 wronly as ext2
On Mon, Jun 30, 2008 at 05:02:50AM +0200, Javier Martín wrote: Here is the patch I was talking about, one step farther than I had first envisioned, i.e. not just about ext4 but an (solid?) implementation of xenophobia in the filesystem driver. This code checks the superblock backwards-incompatible features bitfield against a predefined set of features that we do support, and refuses to mount the filesystem if there are any that we don't. In particular, this makes the driver reject ext4 filesystems with the extents option enabled. I like the idea, but it sounds a bit scary. Is there possibility that GRUB (not grub-probe) refuses to access a filesystem that would otherwise be perfectly usable? -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Tue, 2008-07-01 at 18:08 +0200, Robert Millan wrote: We must not quit if the journal flag is set, even if we don't handle it. grub-setup runs in a active system, the journal wouldn't be empty. If we just quit, we can't even install. I think we should be more conservative here, and only reject a filesystem when we know _for sure_ that GRUB won't be able to access it. Otherwise we may be disabling filesystems that are probably fine. I agree. Rules for read-only access should be more permissive than those for read-write access. Rules for bootloader read-only access should be relaxed even more. For example, we don't really care about permissions and timestamps. It would be nice to get them right, but failure to boot because of a nanosecond timestamp would be too much. Likewise, we don't care if some files are compressed or use a file size representation we don't support and long as the files we need don't use it. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mar, 01-07-2008 a las 12:25 -0400, Pavel Roskin escribió: On Tue, 2008-07-01 at 18:08 +0200, Robert Millan wrote: We must not quit if the journal flag is set, even if we don't handle it. grub-setup runs in a active system, the journal wouldn't be empty. If we just quit, we can't even install. I think we should be more conservative here, and only reject a filesystem when we know _for sure_ that GRUB won't be able to access it. Otherwise we may be disabling filesystems that are probably fine. I agree. Rules for read-only access should be more permissive than those for read-write access. Rules for bootloader read-only access should be relaxed even more. For example, we don't really care about permissions and timestamps. It would be nice to get them right, but failure to boot because of a nanosecond timestamp would be too much. Likewise, we don't care if some files are compressed or use a file size representation we don't support and long as the files we need don't use it. Well, what can I say about this: INCOMPAT_* flags are so for a reason, and they are telling us don't even try to read this filesystem if you don't implement this. It's true that _maybe_ the files we need don't have extents, or compression, or other incompatible things, but then we'd have to strengthen _every other_ routine in the driver, like those that read inodes, guarding them against format changes that we have probably ignored bypassing the incompatible features check. From the POV of correctness I'd prefer to have a single point of failure in the mount routine. Also, as a GRUB user I would find it quite strange that a filesystem that is listed as recognized and whose files can be lsed would not let me access a particular file because (insert unrecognized inode format error here). I _would_ understand such errors if the system showed the partition as unrecognized and then I had to specifically request it to be mounted as ext2 with a possible --ignore-incompatible flag, because then I would be knowingly doing something risky, but the system should not take such kind of decisions on its own unless the GRUB developers _know_ about a particular flag and, after weighing the pros and cons, specifically decide to ignore it (like the proposed patch does with needs_recovery). However, doing that with possibly unknown future flags is a no-go. 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
Re: grub-probe detects ext4 wronly as ext2
On Tue, 2008-07-01 at 20:42 +0200, Javier Martín wrote: Well, what can I say about this: INCOMPAT_* flags are so for a reason, and they are telling us don't even try to read this filesystem if you don't implement this. It's true that _maybe_ the files we need don't have extents, or compression, or other incompatible things, but then we'd have to strengthen _every other_ routine in the driver, like those that read inodes, guarding them against format changes that we have probably ignored bypassing the incompatible features check. From the POV of correctness I'd prefer to have a single point of failure in the mount routine. Also, as a GRUB user I would find it quite strange that a filesystem that is listed as recognized and whose files can be lsed would not let me access a particular file because (insert unrecognized inode format error here). I _would_ understand such errors if the system showed the partition as unrecognized and then I had to specifically request it to be mounted as ext2 with a possible --ignore-incompatible flag, because then I would be knowingly doing something risky, but the system should not take such kind of decisions on its own unless the GRUB developers _know_ about a particular flag and, after weighing the pros and cons, specifically decide to ignore it (like the proposed patch does with needs_recovery). However, doing that with possibly unknown future flags is a no-go. OK, I wasn't arguing against your patch. I just tried to explain why we can relax some criteria compared to what a filesystem driver in a kernel would do. I actually agree with most of your arguments. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Tue, Jul 01, 2008 at 08:42:39PM +0200, Javier Martín wrote: partition as unrecognized and then I had to specifically request it to be mounted as ext2 with a possible --ignore-incompatible flag, A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. Though, if non-essential stuff needs to be implemented, please take into account that we're really pressed for space in ext2.mod (and try to use a separate module for that). -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El mar, 01-07-2008 a las 22:48 +0200, Robert Millan escribió: On Tue, Jul 01, 2008 at 08:42:39PM +0200, Javier Martín wrote: partition as unrecognized and then I had to specifically request it to be mounted as ext2 with a possible --ignore-incompatible flag, A --ignore-incompatible flag doesn't sound like a nice thing to do; it means we're passing our own problem to the user instead of solving it. We don't have any problem to pass to users: ext4 is not supported and thus we do the Right Thing (tm) in patching our ext2 driver so that it won't try to read a filesystem it cannot. However, given Pavel's and others' objections, I suggested the addition of an user override to it. Thus, the user will have to knowingly force the system to interpret the filesystem with its current code, and accept any failures he might get, instead of the current behaviour of having the FS mounted automatically without checking incompatibilities (and then getting the errors anyway). Furthermore, the possibility of accidentally adding an incompatible feature is not exactly high: for an ext3 FS to get one of the ext4 flags, one has to explicitly mount it as ext4dev (usually installing or hand-compiling the module before, because most distros don't include it by default) _and_ create new files on it. Then and only then will the FS gain the extents flag. Though, if non-essential stuff needs to be implemented, please take into account that we're really pressed for space in ext2.mod (and try to use a separate module for that). The proposal (a patch which is essentially under ten lines of code long) could _avoid_ the implementation of format compatibility checks in all the inode-handling functions, since after passing the compatibility check we know the format we'll encounter is at least ro-compatible with our capabilities, or the user is braced for the possible errors. With the current implementation, an unawares user could be flooded by inode format errors becase, for example, an ext4 FS got mounted by our ext2 driver. The override proposal is not implemented in the current patch, but it could be as simple as an environment variable. Consider this case, with (hd0,1) an ext3 /boot partition that was accidentally converted to ext4 and then got a file copied to it (the 2.6.26-rc2 kernel), then gaining the extents flag: grub ls (hd0,1)/ error: unrecognized filesystem grub set ext2_options=ignore_incompatible grub ls (hd0,1)/ kernel-2.6.24-r1 kernel-2.6.26-rc2 grub kernel (hd0,1)/kernel-2.6.26-rc2 root=/dev/sda5 error: file not found # (I dunno what error bad inode is) grub kernel (hd0,1)/kernel-2.6.24 root=/dev/sda5 [Linux-bzimage, 0x10] grub boot 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
Re: grub-probe detects ext4 wronly as ext2
From: JavierMartín [EMAIL PROTECTED] Sent: Monday, June 30, 2008 5:02 AM Phew, that was all (I hope) Cheers! Habbit Thank you for the patch. Just tried it out, it works fine ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
+#define EXT3_FEATURE_INCOMPAT_RECOVER 0x0004 /* Needs recovery */ +#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE ) I suspect this will mean that journalled ext3 when the system crashed (so the filesystem needs recovery from the journal) won't load. (Of course, properly speaking that would load grub's code to replay the journal...) But I think that (without other changes) that would make the system unbootable every time there was a power outage? (Of course it was not guaranteed to load correctly when ignoring the journal when it needed recovery, but it was likely to work, IIUC.) -Isaac ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
On Mon, Jun 30, 2008 at 8:12 PM, Javier Martín [EMAIL PROTECTED] wrote: El lun, 30-06-2008 a las 07:14 -0400, Isaac Dupree escribió: +#define EXT3_FEATURE_INCOMPAT_RECOVER 0x0004 /* Needs recovery */ +#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE ) I suspect this will mean that journalled ext3 when the system crashed (so the filesystem needs recovery from the journal) won't load. (Of course, properly speaking that would load grub's code to replay the journal...) But I think that (without other changes) that would make the system unbootable every time there was a power outage? (Of course it was not guaranteed to load correctly when ignoring the journal when it needed recovery, but it was likely to work, IIUC.) -Isaac As I said, I didn't add it because I didn't know whether recovery was supported or not. _Theoretically_ we should focus on correctness and refuse to read such a filesystem, but here goes a workaround for incompatible features that we do not support but still willingly want to ignore for the sake of compatibility. This new version of the patch adds another macro, EXT2_DRIVER_IGNORED_INCOMPAT where we can put features that we don't fully support, but still want a filesystem with them to be mounted, like the needs_recover flag. Of course, this is risky: INCOMPAT_* features are so for a reason, but it will allow dirty ext3 filesystems to be mounted until we have a working journal implementation. I had thought of adding some kind of warning, but since GRUB mounts and umounts filesystems constantly, it just cluttered the screen and I removed it. Hi, We must not quit if the journal flag is set, even if we don't handle it. grub-setup runs in a active system, the journal wouldn't be empty. If we just quit, we can't even install. -- Bean ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
Hi there, El lun, 30-06-2008 a las 20:27 +0800, Bean escribió: Hi, We must not quit if the journal flag is set, even if we don't handle it. grub-setup runs in a active system, the journal wouldn't be empty. If we just quit, we can't even install. If you mean the needs_recovery flag (EXT3_FEATURE_INCOMPAT_RECOVER), the last version of the patch does with it nicely, as it is added to the set of incompatible feature flags to be ignored in the check. It's not added to the list of supported incompatible features because true support for recovery and replay is not implemented right now, but dirty ext3 filesystems will still be mounted, even if it's risky without having journal replay functionality. Ext4 filesystems, on the other hand, will not, since the incompatible extents feature is neither in the supported nor the ignored list. 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
grub-probe detects ext4 wronly as ext2
Hello, I reported this already on Debian as #488235 grub-probe -t fs shows ext2 for a filesystem created with e2fsprogs 1.41WIP from Debian experimental with extent and flex_bg I didn't get grub-probe working with the loopback device even though I added it to device.map it still complained. The attached image is a 10 mb ext4 made with mkfs.ext4 without any special options just the extent and flex_bg features enabled by mke2fs.conf If I make that on a 100 MB Disk in VMware grub-probe -t fs shows ext2 so I think it shouldn't be different on that 10 MB file ext4.img.bz2 Description: Binary data ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El dom, 29-06-2008 a las 20:11 +0200, Felix Zielcke escribió: Hello, I reported this already on Debian as #488235 grub-probe -t fs shows ext2 for a filesystem created with e2fsprogs 1.41WIP from Debian experimental with extent and flex_bg I didn't get grub-probe working with the loopback device even though I added it to device.map it still complained. The attached image is a 10 mb ext4 made with mkfs.ext4 without any special options just the extent and flex_bg features enabled by mke2fs.conf If I make that on a 100 MB Disk in VMware grub-probe -t fs shows ext2 so I think it shouldn't be different on that 10 MB file ext4 on-disk format is very similar to ext2, but backwards compatibility is broken by the extents option (and iIrc, a modification in the inode format?). Thus, we need to patch our ext2/3 drivers so that they reject mounting a filesystem with the extents feature bit set, as a temporary solution. Then, we can develop an ext4 driver that understands the new format. 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
Re: grub-probe detects ext4 wronly as ext2
On Mon, Jun 30, 2008 at 2:46 AM, Javier Martín [EMAIL PROTECTED] wrote: El dom, 29-06-2008 a las 20:11 +0200, Felix Zielcke escribió: Hello, I reported this already on Debian as #488235 grub-probe -t fs shows ext2 for a filesystem created with e2fsprogs 1.41WIP from Debian experimental with extent and flex_bg I didn't get grub-probe working with the loopback device even though I added it to device.map it still complained. The attached image is a 10 mb ext4 made with mkfs.ext4 without any special options just the extent and flex_bg features enabled by mke2fs.conf If I make that on a 100 MB Disk in VMware grub-probe -t fs shows ext2 so I think it shouldn't be different on that 10 MB file ext4 on-disk format is very similar to ext2, but backwards compatibility is broken by the extents option (and iIrc, a modification in the inode format?). Thus, we need to patch our ext2/3 drivers so that they reject mounting a filesystem with the extents feature bit set, as a temporary solution. Then, we can develop an ext4 driver that understands the new format. Hi, I think it's not difficult to add extents support for grub2. Is the feature stable now, how many distro enable it by default ? -- Bean ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El lun, 30-06-2008 a las 03:17 +0800, Bean escribió: Hi, I think it's not difficult to add extents support for grub2. Is the feature stable now, how many distro enable it by default ? Ext4 is as of today in development and unstable; in fact, the FS name in Linux is ext4dev, but I _think_ the on-disk format is already frozen and thus a readonly driver can be written. I'm not saying that we should delay the implementation of such a driver, just the we first need to address the potentially fatal problem of an ext4 FS mounted as ext3/2. No distro enables ext4 by default. In the case of the 2-3 transition the only thing a readonly ext2 driver missed from the ext3 FS was the journal and the dirindex htrees, which speed up directory indexing. This did not prevent data being read from the FS (though in the case of a crash incorrect metadata could be read), but in the case of ext4, the larger inodes and extent features _do_ prevent reads with an unprepared ext2 driver. The presence of these incompatible changes is signaled with set bits in the ext2 superblock features area (and are listed by tune2fs). Thus, what I propose is a quick fix first, adding a small patch to the ext2/3 driver that would detect such incompatible features and reject mounting the FS (and might be even overridable with an option at module initialization, like -force4). Then we'd be free to implement ext4 without people reporting strange failures as the kernel is loaded, but initrd is not or such. 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
Re: grub-probe detects ext4 wronly as ext2
On Sun, Jun 29, 2008 at 09:53:50PM +0200, Javier Martín wrote: Ext4 is as of today in development and unstable; in fact, the FS name in Linux is ext4dev, but I _think_ the on-disk format is already frozen and thus a readonly driver can be written. I'm not saying that we should delay the implementation of such a driver, just the we first need to address the potentially fatal problem of an ext4 FS mounted as ext3/2. No distro enables ext4 by default. Yep. update-grub heavily relies on grub-probe to figure out if a filesystem will be accessible, and therefore whether to enable optional features. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe detects ext4 wronly as ext2
El dom, 29-06-2008 a las 23:19 +0200, Robert Millan escribió: On Sun, Jun 29, 2008 at 09:53:50PM +0200, Javier Martín wrote: Ext4 is as of today in development and unstable; in fact, the FS name in Linux is ext4dev, but I _think_ the on-disk format is already frozen and thus a readonly driver can be written. I'm not saying that we should delay the implementation of such a driver, just the we first need to address the potentially fatal problem of an ext4 FS mounted as ext3/2. No distro enables ext4 by default. Yep. update-grub heavily relies on grub-probe to figure out if a filesystem will be accessible, and therefore whether to enable optional features. Here is the patch I was talking about, one step farther than I had first envisioned, i.e. not just about ext4 but an (solid?) implementation of xenophobia in the filesystem driver. This code checks the superblock backwards-incompatible features bitfield against a predefined set of features that we do support, and refuses to mount the filesystem if there are any that we don't. In particular, this makes the driver reject ext4 filesystems with the extents option enabled. As I don't know what INCOMPAT_* features are implemented, I've added the one I'm sure we do support because it is used in the code: filetype. However, someone with insight in the ext2 driver should take a look at the patch and add all INCOMPAT_* features that we support to the new define created to that effect: EXT2_DRIVER_SUPPORTED_INCOMPAT. Just OR the new flags with the one in there. Failure to do so might cause regressions if this patch is committed (i.e. FS that mounted fine before will now refuse to do so), but will ensure that we only try to read what we can. I've copied the EXTn_FEATURE_* #defines from the Linux kernel headers, but only two are actually used (filetype and has_journal, which was already there, presumably to detect ext3 filesystems) - the rest are there for completion, but can be removed if you deem it better, though I'd suggest keeping at least the EXTn_FEATURE_INCOMPAT_* macros. This patch has been tested on a qemu virtual machine, with two ext2 partitions that started identical. I mounted one of them as ext4dev with the extents option and copied a new file to it, thus enabling the extents bit in the superblock. Without the patch, GRUB would happily read both partitions as ext2 (I didn't try to read the new file, but most probably that would have caused some havoc). With the patch, the ext4 partition is shown as unknown filesystem. A good changelog entry might be fs/ext2.c: ext2 driver will now reject filesystems with unknown incompatible features. This patch detects only incompatible features, so ext3 devices with internal journal should continue to work as they did. Phew, that was all (I hope) Cheers! Habbit Index: fs/ext2.c === RCS file: /sources/grub/grub2/fs/ext2.c,v retrieving revision 1.26 diff -u -r1.26 ext2.c --- fs/ext2.c 16 Jun 2008 19:02:07 - 1.26 +++ fs/ext2.c 30 Jun 2008 02:36:13 - @@ -71,7 +71,33 @@ ? 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) */ +#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) */ +#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) */ +#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 /* Journal device */ +#define EXT2_FEATURE_INCOMPAT_META_BG 0x0010 +#define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 /* extents support */ +#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 ) #define EXT3_JOURNAL_MAGIC_NUMBER 0xc03b3998U @@ -394,6 +420,11 @@ if (grub_le_to_cpu16 (data-sblock.magic) != EXT2_MAGIC) goto fail; + /* Check the FS doesn't have feature bits enabled that we don't support */ + if (grub_le_to_cpu32 (data-sblock.feature_incompat) +
