grub-probe detects ext4 wronly as ext2

grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

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 

[-- Attachment #2: ext4.img.bz2 --]
[-- Type: application/octet-stream, Size: 1837 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Bean]

On Mon, Jun 30, 2008 at 2:46 AM, Javier Martín <lordhabbit@gmail.com> 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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 2912 bytes --]

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

[-- Attachment #1.2: ext4_unrecognized.diff --]
[-- Type: text/x-patch, Size: 2620 bytes --]

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 -0000	1.26
+++ fs/ext2.c	30 Jun 2008 02:36:13 -0000
@@ -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)
+      & ~EXT2_DRIVER_SUPPORTED_INCOMPAT)
+    goto fail;
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -409,7 +440,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;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

From: "JavierMartín" <lordhabbit@gmail.com>
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 

Re: grub-probe detects ext4 wronly as ext2

[Isaac Dupree]

> +#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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 1611 bytes --]

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.

[-- Attachment #1.2: ext4_unrecognized.diff --]
[-- Type: text/x-patch, Size: 2948 bytes --]

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 -0000	1.26
+++ fs/ext2.c	30 Jun 2008 12:07:56 -0000
@@ -71,7 +71,37 @@
          ? 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
 
@@ -394,6 +424,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)
+      & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    goto fail;
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -409,7 +444,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;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Bean]

On Mon, Jun 30, 2008 at 8:12 PM, Javier Martín <lordhabbit@gmail.com> 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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

On Mon, Jun 30, 2008 at 08:27:50PM +0800, Bean wrote:
> On Mon, Jun 30, 2008 at 8:12 PM, Javier Martín <lordhabbit@gmail.com> 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.

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.

-- 
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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Pavel Roskin]

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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Pavel Roskin]

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

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

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": ext4 is currently not supporting and we do
the Right Thing (tm) in fixing our ext2 driver so that it won't try to
read filesystems it cannot. Then, given Pavel's an others' arguments, I
suggest the addition of such an user-accessible 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).
> 

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

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, 0x100000]
	grub> boot

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Pavel Roskin]

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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 6045 bytes --]

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
unworkable here as there is no prompt.
Of course, there are INCOMPAT_* flags that we know are not really such
(like needs_recovery). We can add those flags to the ignore list and
just forget about them, or implement some handling of them, like
replaying the journal in memory; but we cannot transparently ignore all
INCOMPAT_* flags because we don't know the future!

Thus, I agree with you that grub-probe should adopt the most
conservative stance possible, but reject the idea that the bootloader
proper should automatically activate "best-effort" reading, since this
can lead to very strange and untraceable bugs in the future (I'm just
imagining the puzzled look on the osdev's face when his perfect
multiboot kernel on the ext5 filesystem triple-faults without apparent
motive). Thus, the default should be conservative reading, keeping
best-effort reading to be enabled through an user override.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 xxxxx ; 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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Isaac Dupree]

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 xxxxx ; 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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 5175 bytes --]

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 xxxxx ; 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.


[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 6858 bytes --]

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_ make a difference because we can avoid it
being our fault if we just obey the huge warning signs in the
superblock! If the user specifically requests us to try and read the
partition even though the superblock tells us not to, then we proceed
and whatever happens is, at least in part, his fault, even though this
does not completely exonerate us for not having a completely up-to-date
filesystem driver.

> > I'm finding this discussion increasingly unproductive
> 
> Hey, at least we agree on something :-)
> 
As I threatened in my last post, here is the full patch, with the
incompatible features detection and the user override option, for a
total of +168 bytes in i386. It works like this (test was on a QEMU VM,
with GRUB booting from floppy and hda1 being an ext2 part turned ext4):
	grub> ls (hd0,1)/
	error: unknown filesystem
	grub> set ext2_options=ignore_incompatible
	grub> ls (hd0,1)/
	lost+found/ grub/ Godfather.wav vmlinuz-2.6.24-16-server (&more)
	grub> linux (hd0,1)/vmlinuz-2.6.24-16-server
		[Linux-bzImage, setup=0x2a00, size=0x1e26d8]
	grub> hexdump (hd0,1)/Godfather.wav
	(shows 256 bytes of zeros instead of the actual contents)
Of course, the new file (with extents) was the Godfather tune, while the
kernel was an old extentless file. Trying to "play" the file crashed
QEMU (I suspect that meant a triple fault happened in the simulation?)

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).

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[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 :)

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

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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/msg00008.html
[1] http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00333.html

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

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/msg00008.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

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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/msg00008.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 :)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 68 bytes --]

And, after writing all that, I forgot to append the patch. Sigh.


[-- Attachment #1.2: ext4_unrecognized.patch --]
[-- Type: text/x-patch, Size: 3657 bytes --]

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;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Bean]

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

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

From: "Bean" <bean123ch@gmail.com>
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 :) 

[-- Attachment #2: strace.txt --]
[-- Type: text/plain, Size: 30610 bytes --]

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", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 8587116032, SEEK_SET)          = 8587116032
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 8587120128, SEEK_SET)          = 8587120128
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/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed9478)  = 0
close(3)                                = 0
open("/dev/sdb", 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/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75eda488)  = 0
close(3)                                = 0
open("/dev/sdb", O_RDONLY|O_SYNC)       = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 0, SEEK_SET)                   = 0
read(3, "\353L\220\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, "RV\276\33\201\350@\1^\277\364\201f\213-\203}\10\0\17\204"..., 3584)
= 3584
close(3)                                = 0
open("/dev/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed8ff8)  = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 32764416, SEEK_SET)            = 32764416
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/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 32768512, SEEK_SET)            = 32768512
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/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed8ff8)  = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 8554215424, SEEK_SET)          = 8554215424
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/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed8f60)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 8554219520, SEEK_SET)          = 8554219520
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, 0x7fff75eda1d8)  = 0
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, 0x7fff75eda498)  = 0
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, 0x7fff75ed9d58)  = 0
close(3)                                = 0
open("/dev/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75eda1d8)  = 0
close(3)                                = 0
open("/dev/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75eda498)  = 0
close(3)                                = 0
open("/dev/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed9d58)  = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 18446744073709548032, SEEK_SET) = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb1", 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
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 512, SEEK_SET)                 = 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"..., 4096) =
4096
close(3)                                = 0
open("/dev/sdb", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 16), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75ed9d58)  = 0
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 18446744073709549568, SEEK_SET) = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", 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
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 18446744073709549568, SEEK_SET) = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 512, SEEK_SET)                 = 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"..., 512) =
512
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 18446744073709549568, SEEK_SET) = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 1024, SEEK_SET)                = 1024
read(3, "\0\370\7\0\2\336\37\0\346\227\1\0\234\267\34\0\32\251\7"..., 512) =
512
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 18446744073709549568, SEEK_SET) = -1 EINVAL (Invalid argument)
close(3)                                = 0
open("/dev/sdb1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75ed9cc0)         = 0
close(3)                                = 0
open("/dev/sdb2", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 1536, SEEK_SET)                = 1536
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
close(3)                                = 0
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
open("/dev/sda", O_RDONLY)              = 3
ioctl(3, 0x301, 0x7fff75eda580)         = 0
close(3)                                = 0
stat("/boot/grub", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
open("/dev/mapper", O_RDONLY|O_NONBLOCK|O_DIRECTORY|0x80000) = -1 ENOENT (No
such file or directory)
open("/dev/evms", O_RDONLY|O_NONBLOCK|O_DIRECTORY|0x80000) = -1 ENOENT (No
such file or directory)
open("/dev", O_RDONLY|O_NONBLOCK|O_DIRECTORY|0x80000) = 3
fstat(3, {st_mode=S_IFDIR|0755, st_size=2580, ...}) = 0
fcntl(3, F_GETFD)                       = 0x1 (flags FD_CLOEXEC)
getcwd("/root", 10)                     = 6
chdir("/dev")                           = 0
getdents(3, /* 129 entries */, 4096)    = 3752
lstat("vcsa6", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 134), ...}) = 0
lstat("vcs6", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 6), ...}) = 0
lstat("vcsa5", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 133), ...}) = 0
lstat("vcs5", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 5), ...}) = 0
lstat("vcsa4", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 132), ...}) = 0
lstat("vcs4", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 4), ...}) = 0
lstat("vcsa3", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 131), ...}) = 0
lstat("vcs3", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 3), ...}) = 0
lstat("vcsa2", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 130), ...}) = 0
lstat("vcs2", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 2), ...}) = 0
lstat("vcsa1", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 129), ...}) = 0
lstat("vcs1", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 1), ...}) = 0
lstat("vmci", {st_mode=S_IFCHR|0600, st_rdev=makedev(254, 0), ...}) = 0
lstat("log", {st_mode=S_IFSOCK|0666, st_size=0, ...}) = 0
lstat("xconsole", {st_mode=S_IFIFO|0640, st_size=0, ...}) = 0
lstat("cdrom", {st_mode=S_IFLNK|0777, st_size=3, ...}) = 0
lstat("hda", {st_mode=S_IFBLK|0660, st_rdev=makedev(3, 0), ...}) = 0
lstat("vcsa", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 128), ...}) = 0
lstat("vcs", {st_mode=S_IFCHR|0660, st_rdev=makedev(7, 0), ...}) = 0
lstat("tty9", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 9), ...}) = 0
lstat("tty8", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 8), ...}) = 0
lstat("tty7", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 7), ...}) = 0
lstat("tty63", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 63), ...}) = 0
lstat("tty62", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 62), ...}) = 0
lstat("tty61", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 61), ...}) = 0
lstat("tty60", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 60), ...}) = 0
lstat("tty6", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 6), ...}) = 0
lstat("tty59", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 59), ...}) = 0
lstat("tty58", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 58), ...}) = 0
lstat("tty57", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 57), ...}) = 0
lstat("tty56", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 56), ...}) = 0
lstat("tty55", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 55), ...}) = 0
lstat("tty54", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 54), ...}) = 0
lstat("tty53", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 53), ...}) = 0
lstat("tty52", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 52), ...}) = 0
lstat("tty51", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 51), ...}) = 0
lstat("tty50", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 50), ...}) = 0
lstat("tty5", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 5), ...}) = 0
lstat("tty49", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 49), ...}) = 0
lstat("tty48", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 48), ...}) = 0
lstat("tty47", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 47), ...}) = 0
lstat("tty46", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 46), ...}) = 0
lstat("tty45", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 45), ...}) = 0
lstat("tty44", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 44), ...}) = 0
lstat("tty43", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 43), ...}) = 0
lstat("tty42", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 42), ...}) = 0
lstat("tty41", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 41), ...}) = 0
lstat("tty40", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 40), ...}) = 0
lstat("tty4", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 4), ...}) = 0
lstat("tty39", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 39), ...}) = 0
lstat("tty38", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 38), ...}) = 0
lstat("tty37", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 37), ...}) = 0
lstat("tty36", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 36), ...}) = 0
lstat("tty35", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 35), ...}) = 0
lstat("tty34", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 34), ...}) = 0
lstat("tty33", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 33), ...}) = 0
lstat("tty32", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 32), ...}) = 0
lstat("tty31", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 31), ...}) = 0
lstat("tty30", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 30), ...}) = 0
lstat("tty3", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 3), ...}) = 0
lstat("tty29", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 29), ...}) = 0
lstat("tty28", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 28), ...}) = 0
lstat("tty27", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 27), ...}) = 0
lstat("tty26", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 26), ...}) = 0
lstat("tty25", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 25), ...}) = 0
lstat("tty24", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 24), ...}) = 0
lstat("tty23", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 23), ...}) = 0
lstat("tty22", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 22), ...}) = 0
lstat("tty21", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 21), ...}) = 0
lstat("tty20", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 20), ...}) = 0
lstat("tty2", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 2), ...}) = 0
lstat("tty19", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 19), ...}) = 0
lstat("tty18", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 18), ...}) = 0
lstat("tty17", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 17), ...}) = 0
lstat("tty16", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 16), ...}) = 0
lstat("tty15", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 15), ...}) = 0
lstat("initctl", {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0
lstat("tty14", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 14), ...}) = 0
lstat("tty13", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 13), ...}) = 0
lstat("tty12", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 12), ...}) = 0
lstat("tty11", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 11), ...}) = 0
lstat("tty10", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 10), ...}) = 0
lstat("tty1", {st_mode=S_IFCHR|0600, st_rdev=makedev(4, 1), ...}) = 0
lstat("tty0", {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 0), ...}) = 0
lstat("tty", {st_mode=S_IFCHR|0666, st_rdev=makedev(5, 0), ...}) = 0
lstat("ptmx", {st_mode=S_IFCHR|0666, st_rdev=makedev(5, 2), ...}) = 0
lstat("rtc", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 135), ...}) = 0
lstat("network_throughput", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 61),
...}) = 0
lstat("network_latency", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 62),
...}) = 0
lstat("mcelog", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 227), ...}) = 0
lstat("hpet", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 228), ...}) = 0
lstat("cpu_dma_latency", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 63),
...}) = 0
lstat("agpgart", {st_mode=S_IFCHR|0660, st_rdev=makedev(10, 175), ...}) = 0
lstat("zero", {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 5), ...}) = 0
lstat("urandom", {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 9), ...}) = 0
lstat("random", {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 8), ...}) = 0
lstat("port", {st_mode=S_IFCHR|0640, st_rdev=makedev(1, 4), ...}) = 0
lstat("input", {st_mode=S_IFDIR|0755, st_size=60, ...}) = 0
open("input", O_RDONLY|O_NONBLOCK|O_DIRECTORY|0x80000) = 4
fstat(4, {st_mode=S_IFDIR|0755, st_size=60, ...}) = 0
getcwd("/dev", 10)                      = 5
chdir("input")                          = 0
getdents(4, /* 3 entries */, 4096)      = 72
lstat("mice", {st_mode=S_IFCHR|0660, st_rdev=makedev(13, 63), ...}) = 0
getdents(4, /* 0 entries */, 4096)      = 0
chdir("/dev")                           = 0
close(4)                                = 0
lstat("mem", {st_mode=S_IFCHR|0640, st_rdev=makedev(1, 1), ...}) = 0
lstat("kmsg", {st_mode=S_IFCHR|0660, st_rdev=makedev(1, 11), ...}) = 0
lstat("full", {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 7), ...}) = 0
lstat("fb0", {st_mode=S_IFCHR|0660, st_rdev=makedev(29, 0), ...}) = 0
lstat("sdc1", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 33), ...}) = 0
lstat("sdc2", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 34), ...}) = 0
lstat("sdb1", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 17), ...}) = 0
lstat("sdb2", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 18), ...}) = 0
lstat("sdc", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 32), ...}) = 0
lstat("sda1", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 1), ...}) = 0
getcwd("/dev", 10)                      = 5
chdir("/root")                          = 0
close(3)                                = 0
stat("/dev/sda1", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 1), ...}) = 0
lstat("/dev", {st_mode=S_IFDIR|0755, st_size=2580, ...}) = 0
lstat("/dev/sda1", {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 1), ...}) = 0
open("/dev/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda580)         = 0
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
stat("/dev/mapper/hd0,1", 0x7fff75eda530) = -1 ENOENT (No such file or
directory)
stat("/boot/grub/boot.img", {st_mode=S_IFREG|0644, st_size=512, ...}) = 0
stat("/boot/grub/boot.img", {st_mode=S_IFREG|0644, st_size=512, ...}) = 0
open("/boot/grub/boot.img", O_RDONLY)   = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=512, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded5000
lseek(3, 0, SEEK_SET)                   = 0
read(3, "\353L\220\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096)
= 512
close(3)                                = 0
munmap(0x7ff16ded5000, 4096)            = 0
stat("/boot/grub/core.img", {st_mode=S_IFREG|0644, st_size=27980, ...}) = 0
stat("/boot/grub/core.img", {st_mode=S_IFREG|0644, st_size=27980, ...}) = 0
open("/boot/grub/core.img", O_RDONLY)   = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=27980, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ff16ded5000
lseek(3, 0, SEEK_SET)                   = 0
read(3, "RV\276\33\201\350@\1^\277\364\201f\213-\203}\10\0\17\204"...,
24576) = 24576
read(3, "\10\271%\335A\301|\1AI\321\336\4\326\2\275\22\335\336\7"..., 4096)
= 3404
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, 0x7fff75eda468)  = 0
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, 0x7fff75eda468)  = 0
close(3)                                = 0
sync()                                  = 0
sync()                                  = 0
open("/dev/sda", O_RDONLY)              = 3
fstat(3, {st_mode=S_IFBLK|0660, st_rdev=makedev(8, 0), ...}) = 0
ioctl(3, BLKGETSIZE64, 0x7fff75eda438)  = 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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda330)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 65024, SEEK_SET)               = 65024
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda330)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 7680, SEEK_SET)                = 7680
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda330)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 261632, SEEK_SET)              = 261632
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda150)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 32256, SEEK_SET)               = 32256
read(3, "\377\377\377\377\377\377\377\377\377\377\377\377\377\377"..., 4096)
= 4096
close(3)                                = 0
open("/dev/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda2d0)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 3584, SEEK_SET)                = 3584
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda2d0)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 142848, SEEK_SET)              = 142848
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda250)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 73216, SEEK_SET)               = 73216
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda1b0)         = 0
close(3)                                = 0
open("/dev/sda1", O_RDONLY|O_SYNC)      = 3
ioctl(3, BLKFLSBUF, 0)                  = 0
lseek(3, 6444682752, SEEK_SET)          = 6444682752
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/sda1", O_RDONLY)             = 3
ioctl(3, 0x301, 0x7fff75eda190)         = 0
close(3)                                = 0
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)                                = 0

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]

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?

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Bean]

On Fri, Jul 4, 2008 at 6:34 PM, Javier Martín <lordhabbit@gmail.com> 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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 5975 bytes --]

El vie, 04-07-2008 a las 19:29 +0800, Bean escribió:
> On Fri, Jul 4, 2008 at 6:34 PM, Javier Martín <lordhabbit@gmail.com> 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 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.
> 
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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Bean]

On Fri, Jul 4, 2008 at 8:00 PM, Javier Martín <lordhabbit@gmail.com> wrote:
> El vie, 04-07-2008 a las 19:29 +0800, Bean escribió:
>> On Fri, Jul 4, 2008 at 6:34 PM, Javier Martín <lordhabbit@gmail.com> 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 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.
>>
> 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.

Hi,

Actually, I don't see the point of this discussion. The worst case
scenario is hanging grub, instead of report a unknown file system
error. The end result the same, user can't boot from this partition.
The point here is that we should try our best to read from it. If it's
indeed incompatible, there is nothing we can do.

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. 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 ?

-- 
Bean

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 1530 bytes --]

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?

[-- Attachment #1.2: ext4_unrecognized.patch --]
[-- Type: text/x-patch, Size: 2873 bytes --]

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;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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_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

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 2550 bytes --]

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".

[-- Attachment #1.2: ext2_incompat.patch.3 --]
[-- Type: text/x-patch, Size: 4718 bytes --]

Index: fs/ext2.c
===================================================================
--- fs/ext2.c	(revisión: 1691)
+++ fs/ext2.c	(copia de trabajo)
@@ -71,8 +71,53 @@
          ? EXT2_GOOD_OLD_INODE_SIZE \
          : grub_le_to_cpu16 (data->sblock.inode_size))
 
-#define EXT3_FEATURE_COMPAT_HAS_JOURNAL	0x0004
+/* Superblock filesystem feature flags (RW compatible)
+ * A filesystem with any of these enabled can be read and written by a driver
+ * that does not understand them without causing metadata/data corruption */
+#define EXT2_FEATURE_COMPAT_DIR_PREALLOC	0x0001
+#define EXT2_FEATURE_COMPAT_IMAGIC_INODES	0x0002
+#define EXT3_FEATURE_COMPAT_HAS_JOURNAL		0x0004
+#define EXT2_FEATURE_COMPAT_EXT_ATTR		0x0008
+#define EXT2_FEATURE_COMPAT_RESIZE_INODE	0x0010
+#define EXT2_FEATURE_COMPAT_DIR_INDEX		0x0020
+/* Superblock filesystem feature flags (RO compatible)
+ * A filesystem with any of these enabled can be safely read by a driver that
+ * does not understand them, but should not be written to, usually because
+ * additional metadata is required */
+#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
+#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE	0x0002
+#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR	0x0004
+#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
+#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
+#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+/* Superblock filesystem feature flags (back-incompatible)
+ * A filesystem with any of these enabled should not be attempted to be read
+ * by a driver that does not understand them, since they usually indicate
+ * metadata format changes that might confuse the reader. */
+#define EXT2_FEATURE_INCOMPAT_COMPRESSION	0x0001
+#define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
+#define EXT3_FEATURE_INCOMPAT_RECOVER		0x0004 /* Needs recovery */
+#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV	0x0008 /* Volume is journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG		0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS		0x0040 /* Extents used */
+#define EXT4_FEATURE_INCOMPAT_64BIT		0x0080
+#define EXT4_FEATURE_INCOMPAT_FLEX_BG		0x0200
 
+/* The set of back-incompatible features this driver DOES support. Add (OR)
+ * flags here as the related features are implemented into the driver */
+#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE \
+                                       | EXT4_FEATURE_INCOMPAT_EXTENTS )
+/* List of rationales for the ignored "incompatible" features:
+ * needs_recovery: Not really back-incompatible - was added as such to forbid
+ *                 ext2 drivers from mounting an ext3 volume with a dirty
+ *                 journal because they will ignore the journal, but the next
+ *                 ext3 driver to mount the volume will find the journal and
+ *                 replay it, potentially corrupting the metadata written by
+ *                 the ext2 drivers
+ */
+#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )
+
+
 #define EXT3_JOURNAL_MAGIC_NUMBER	0xc03b3998U
 
 #define EXT3_JOURNAL_DESCRIPTOR_BLOCK	1
@@ -375,10 +420,12 @@
   return 0;
 }
 
+#define EXT2_DRIVER_MOUNT_FAIL(message) { err_msg = (message); goto fail; }
 static struct grub_ext2_data *
 grub_ext2_mount (grub_disk_t disk)
 {
   struct grub_ext2_data *data;
+  const char *err_msg = 0;
 
   data = grub_malloc (sizeof (struct grub_ext2_data));
   if (!data)
@@ -388,12 +435,18 @@
   grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
                   (char *) &data->sblock);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
 
   /* Make sure this is an ext2 filesystem.  */
   if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem (superblock magic mismatch)")
   
+  /* Check the FS doesn't have feature bits enabled that we don't support. */
+  if (grub_le_to_cpu32 (data->sblock.feature_incompat)
+        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features")
+    
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -404,12 +457,14 @@
 
   grub_ext2_read_inode (data, 2, data->inode);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the root directory inode")
   
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  if (!err_msg)
+    err_msg = "DEBUG: mount failed but no error message supplied!";
+  grub_error (GRUB_ERR_BAD_FS, err_msg);
   grub_free (data);
   return 0;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 2619 bytes --]

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


[-- Attachment #1.2: ext2_incompat.patch.4 --]
[-- Type: text/x-patch, Size: 4690 bytes --]

Index: fs/ext2.c
===================================================================
--- fs/ext2.c	(revisión: 1691)
+++ fs/ext2.c	(copia de trabajo)
@@ -71,8 +71,53 @@
          ? EXT2_GOOD_OLD_INODE_SIZE \
          : grub_le_to_cpu16 (data->sblock.inode_size))
 
-#define EXT3_FEATURE_COMPAT_HAS_JOURNAL	0x0004
+/* Superblock filesystem feature flags (RW compatible)
+ * A filesystem with any of these enabled can be read and written by a driver
+ * that does not understand them without causing metadata/data corruption */
+#define EXT2_FEATURE_COMPAT_DIR_PREALLOC	0x0001
+#define EXT2_FEATURE_COMPAT_IMAGIC_INODES	0x0002
+#define EXT3_FEATURE_COMPAT_HAS_JOURNAL		0x0004
+#define EXT2_FEATURE_COMPAT_EXT_ATTR		0x0008
+#define EXT2_FEATURE_COMPAT_RESIZE_INODE	0x0010
+#define EXT2_FEATURE_COMPAT_DIR_INDEX		0x0020
+/* Superblock filesystem feature flags (RO compatible)
+ * A filesystem with any of these enabled can be safely read by a driver that
+ * does not understand them, but should not be written to, usually because
+ * additional metadata is required */
+#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
+#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE	0x0002
+#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR	0x0004
+#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
+#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
+#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+/* Superblock filesystem feature flags (back-incompatible)
+ * A filesystem with any of these enabled should not be attempted to be read
+ * by a driver that does not understand them, since they usually indicate
+ * metadata format changes that might confuse the reader. */
+#define EXT2_FEATURE_INCOMPAT_COMPRESSION	0x0001
+#define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
+#define EXT3_FEATURE_INCOMPAT_RECOVER		0x0004 /* Needs recovery */
+#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV	0x0008 /* Volume is journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG		0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS		0x0040 /* Extents used */
+#define EXT4_FEATURE_INCOMPAT_64BIT		0x0080
+#define EXT4_FEATURE_INCOMPAT_FLEX_BG		0x0200
 
+/* The set of back-incompatible features this driver DOES support. Add (OR)
+ * flags here as the related features are implemented into the driver */
+#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE \
+                                       | EXT4_FEATURE_INCOMPAT_EXTENTS )
+/* List of rationales for the ignored "incompatible" features:
+ * needs_recovery: Not really back-incompatible - was added as such to forbid
+ *                 ext2 drivers from mounting an ext3 volume with a dirty
+ *                 journal because they will ignore the journal, but the next
+ *                 ext3 driver to mount the volume will find the journal and
+ *                 replay it, potentially corrupting the metadata written by
+ *                 the ext2 drivers
+ */
+#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )
+
+
 #define EXT3_JOURNAL_MAGIC_NUMBER	0xc03b3998U
 
 #define EXT3_JOURNAL_DESCRIPTOR_BLOCK	1
@@ -375,10 +420,12 @@
   return 0;
 }
 
+#define EXT2_DRIVER_MOUNT_FAIL(message) { err_msg = (message); goto fail; }
 static struct grub_ext2_data *
 grub_ext2_mount (grub_disk_t disk)
 {
   struct grub_ext2_data *data;
+  const char *err_msg = 0;
 
   data = grub_malloc (sizeof (struct grub_ext2_data));
   if (!data)
@@ -388,12 +435,18 @@
   grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
                   (char *) &data->sblock);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
 
   /* Make sure this is an ext2 filesystem.  */
   if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem")
   
+  /* Check the FS doesn't have feature bits enabled that we don't support. */
+  if (grub_le_to_cpu32 (data->sblock.feature_incompat)
+        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features")
+    
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -404,12 +457,14 @@
 
   grub_ext2_read_inode (data, 2, data->inode);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the root directory inode")
   
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  if (!err_msg)
+    err_msg = "DEBUG: mount failed but no error message supplied!";
+  grub_error (GRUB_ERR_BAD_FS, err_msg);
   grub_free (data);
   return 0;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

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.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

From: "JavierMartín" <lordhabbit@gmail.com>
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 

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

El mié, 16-07-2008 a las 17:27 +0200, Felix Zielcke escribió:
> From: "JavierMartín" <lordhabbit@gmail.com>
> 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.


[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

From: "JavierMartín" <lordhabbit@gmail.com>
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 

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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 

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

> 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
 

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

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.


[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

From: "JavierMartín" <lordhabbit@gmail.com>
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 

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

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 <rmh@aybabtu.com>:
> 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
>

[-- Attachment #2: ext2_incompat.patch.6 --]
[-- Type: application/octet-stream, Size: 4686 bytes --]

Index: fs/ext2.c
===================================================================
--- fs/ext2.c	(revisión: 1799)
+++ 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)
+        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features");
+    
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -514,13 +567,15 @@
   data->inode = &data->diropen.inode;
 
   grub_ext2_read_inode (data, 2, data->inode);
-  if (grub_errno)
-    goto fail;
+  if (grub_errno != GRUB_ERR_NONE)
+    EXT2_DRIVER_MOUNT_FAIL(0);
   
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  /* Only call grub_error if the fail was _not_ caused by underlying errors.  */
+  if (local_error)
+    grub_error (GRUB_ERR_BAD_FS, local_error);
   grub_free (data);
   return 0;
 }

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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.

>  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.

> -  if (grub_errno)
> +  if (grub_errno != GRUB_ERR_NONE)

Why?

> -    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.

> +  /* 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.

-- 
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."

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 3751 bytes --]

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

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

Well, seeing that Robert is back (and what a torrent of activity!), I'll
shamelessly try to resubmit my last post from nearly a month ago in the
hope that he'll notice it.

El sáb, 30-08-2008 a las 23:28 +0200, Javier Martín escribió:
> 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

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]

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.

[-- Attachment #1.2: ext2_incompat.patch.7 --]
[-- Type: text/x-patch, Size: 4800 bytes --]

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)
+        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features");
+    
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -514,13 +567,15 @@
   data->inode = &data->diropen.inode;
 
   grub_ext2_read_inode (data, 2, data->inode);
-  if (grub_errno)
-    goto fail;
+  if (grub_errno != GRUB_ERR_NONE)
+    EXT2_DRIVER_MOUNT_FAIL(0);
   
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  /* Only call grub_error if the fail was _not_ caused by underlying errors.  */
+  if (local_error)
+    grub_error (GRUB_ERR_BAD_FS, local_error);
   grub_free (data);
   return 0;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Felix Zielcke]

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

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 1703 bytes --]

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" ;)

[-- Attachment #1.2: ext2_incompat_flexbg.patch --]
[-- Type: text/x-patch, Size: 2884 bytes --]

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 )
 
 

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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."

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

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 /.)