Discussion:
grub-probe detects ext4 wronly as ext2
(too old to reply)
Felix Zielcke
2008-06-29 18:11:56 UTC
Permalink
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
Javier Martín
2008-06-29 18:46:18 UTC
Permalink
Post by Felix Zielcke
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.
Bean
2008-06-29 19:17:17 UTC
Permalink
Post by Javier Martín
Post by Felix Zielcke
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
Javier Martín
2008-06-29 19:53:50 UTC
Permalink
Post by Bean
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.
Robert Millan
2008-06-29 21:19:57 UTC
Permalink
Post by Javier Martín
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 /.)
Javier Martín
2008-06-30 03:02:50 UTC
Permalink
Post by Robert Millan
Post by Javier Martín
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
Felix Zielcke
2008-06-30 07:10:21 UTC
Permalink
From: "JavierMartín" <***@gmail.com>
Sent: Monday, June 30, 2008 5:02 AM
Post by Javier Martín
Phew, that was all (I hope)
Cheers!
Habbit
Thank you for the patch. Just tried it out, it works fine
Isaac Dupree
2008-06-30 11:14:31 UTC
Permalink
+#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
Javier Martín
2008-06-30 12:12:17 UTC
Permalink
Post by 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
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.
Bean
2008-06-30 12:27:50 UTC
Permalink
Post by Javier Martín
Post by 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
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
Javier Martín
2008-06-30 12:43:51 UTC
Permalink
Hi there,
Post by Bean
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.
Robert Millan
2008-07-01 16:08:27 UTC
Permalink
Post by Bean
Post by Javier Martín
Post by 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
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 /.)
Pavel Roskin
2008-07-01 16:25:45 UTC
Permalink
Post by Robert Millan
Post by Bean
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
Javier Martín
2008-07-01 18:42:39 UTC
Permalink
Post by Pavel Roskin
Post by Robert Millan
Post by Bean
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
Pavel Roskin
2008-07-01 19:01:13 UTC
Permalink
Post by Javier Martín
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.
Robert Millan
2008-07-01 20:48:16 UTC
Permalink
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 /.)
Javier Martín
2008-07-01 23:05:20 UTC
Permalink
Post by Robert Millan
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
Post by Robert Millan
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).
Javier Martín
2008-07-01 23:28:47 UTC
Permalink
Post by Robert Millan
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.
Post by Robert Millan
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
Robert Millan
2008-07-02 14:22:45 UTC
Permalink
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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).
Post by Javier Martín
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 /.)
Pavel Roskin
2008-07-02 16:03:20 UTC
Permalink
Post by Robert Millan
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.
Post by Robert Millan
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
Javier Martín
2008-07-02 19:32:40 UTC
Permalink
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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.
Robert Millan
2008-07-03 14:02:11 UTC
Permalink
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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.
Post by Javier Martín
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 /.)
Isaac Dupree
2008-07-03 14:21:49 UTC
Permalink
Post by Robert Millan
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
- 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
Javier Martín
2008-07-03 17:07:55 UTC
Permalink
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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)".
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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.
Post by Robert Millan
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.

Post by Robert Millan
- 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.
Robert Millan
2008-07-04 00:08:29 UTC
Permalink
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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.
Post by Javier Martín
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.
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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).
Post by Javier Martín
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 /.)
Javier Martín
2008-07-04 01:20:32 UTC
Permalink
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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).
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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!
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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).
Felix Zielcke
2008-08-05 17:23:15 UTC
Permalink
Post by 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
Felix Zielcke
2008-08-06 10:36:41 UTC
Permalink
Post by Felix Zielcke
Post by 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
Javier Martín
2008-08-11 00:35:09 UTC
Permalink
Post by Felix Zielcke
Post by Felix Zielcke
Post by 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
Post by Felix Zielcke
_______________________________________________
Grub-devel mailing list
http://lists.gnu.org/mailman/listinfo/grub-devel
Felix Zielcke
2008-08-11 07:56:57 UTC
Permalink
Post by Javier Martín
Post by Felix Zielcke
[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 :)
Javier Martín
2008-07-04 01:32:43 UTC
Permalink
And, after writing all that, I forgot to append the patch. Sigh.
Bean
2008-07-04 06:49:10 UTC
Permalink
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
Felix Zielcke
2008-07-04 08:33:59 UTC
Permalink
From: "Bean" <***@gmail.com>
Sent: Friday, July 04, 2008 8:49 AM
Post by Bean
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 :)
Javier Martín
2008-07-04 10:34:50 UTC
Permalink
I know I promised not to keep posting but... I just cannot xD
Post by Bean
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.
Post by Bean
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).
Post by Bean
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".
Post by Bean
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?
Bean
2008-07-04 11:29:14 UTC
Permalink
Post by Javier Martín
I know I promised not to keep posting but... I just cannot xD
Post by Bean
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.
Post by Javier Martín
Post by Bean
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.
Post by Javier Martín
Post by Bean
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.
Post by Javier Martín
Post by Bean
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
Javier Martín
2008-07-04 12:00:34 UTC
Permalink
Post by Bean
Post by Javier Martín
I know I promised not to keep posting but... I just cannot xD
Post by Bean
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.
Post by Javier Martín
Post by Bean
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
Robert Millan
2008-07-04 14:09:59 UTC
Permalink
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 /.)
Javier Martín
2008-07-04 14:33:50 UTC
Permalink
Post by Robert Millan
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.
Bean
2008-07-04 14:11:42 UTC
Permalink
Post by Javier Martín
Post by Bean
Post by Javier Martín
I know I promised not to keep posting but... I just cannot xD
Post by Bean
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.
Post by Javier Martín
Post by Bean
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.
Javier Martín
2008-07-04 14:34:56 UTC
Permalink
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.
Robert Millan
2008-07-04 14:04:56 UTC
Permalink
Post by Javier Martín
Post by Bean
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.
Post by Javier Martín
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 /.)
Robert Millan
2008-07-04 14:23:52 UTC
Permalink
Post by Robert Millan
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 /.)
Robert Millan
2008-07-04 14:21:25 UTC
Permalink
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.
- 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 /.)
Javier Martín
2008-07-04 14:45:02 UTC
Permalink
Post by 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.
Ok, here is the old version of the patch without the user override
thingy (and the extents reference removed).
Post by Robert Millan
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)
Post by Robert Millan
- 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?
Robert Millan
2008-07-04 18:57:23 UTC
Permalink
Post by Javier Martín
By the way, I'm already using SVN (and thus svn diff) for this patch. Is
that right? Was the migration completed already?
Yep.
Post by Javier Martín
+/* 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).
Post by Javier Martín
+/* 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).
Post by Javier Martín
+/* 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.
Post by Javier Martín
+ /* 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;
[...]
- 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 /.)
Javier Martín
2008-07-04 20:41:35 UTC
Permalink
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.
Post by Robert Millan
Post by Javier Martín
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.
Post by Robert Millan
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
Post by Robert Millan
Post by Javier Martín
+/* 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).
Post by Robert Millan
Post by Javier Martín
+/* 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"
Post by Robert Millan
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".
Robert Millan
2008-07-05 12:07:57 UTC
Permalink
Post by Javier Martín
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) ;-)
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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.
Post by Javier Martín
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.
Post by Javier Martín
/* 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.
Post by Javier Martín
+ /* 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.
Post by Javier Martín
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 /.)
Javier Martín
2008-07-05 18:36:13 UTC
Permalink
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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 *".
Post by Robert Millan
Post by Javier Martín
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?
Post by Robert Millan
Post by Javier Martín
/* 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
Javier Martín
2008-07-16 15:09:05 UTC
Permalink
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.
Felix Zielcke
2008-07-16 15:27:28 UTC
Permalink
From: "JavierMartín" <***@gmail.com>
Sent: Wednesday, July 16, 2008 5:09 PM
To: "The development of GRUB 2" <grub-***@gnu.org>
Subject: Re: grub-probe detects ext4 wronly as ext2
Post by Javier Martín
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
Javier Martín
2008-07-16 16:38:09 UTC
Permalink
Post by Felix Zielcke
Sent: Wednesday, July 16, 2008 5:09 PM
Subject: Re: grub-probe detects ext4 wronly as ext2
Post by Javier Martín
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.
Post by Felix Zielcke
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
Post by Felix Zielcke
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).
Post by Felix Zielcke
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.
Felix Zielcke
2008-07-16 17:13:05 UTC
Permalink
From: "JavierMartín" <***@gmail.com>
Sent: Wednesday, July 16, 2008 6:38 PM
To: "The development of GRUB 2" <grub-***@gnu.org>
Subject: Re: grub-probe detects ext4 wronly as ext2
Post by Javier Martín
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
Post by Javier Martín
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
Post by Javier Martín
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
Post by Javier Martín
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
Felix Zielcke
2008-07-16 17:21:27 UTC
Permalink
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
Felix Zielcke
2008-07-16 17:44:24 UTC
Permalink
Post by Felix Zielcke
Oh well I should have used grep with -i
meta_bg and META_BG does make a difference
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
Javier Martín
2008-07-16 19:07:21 UTC
Permalink
Post by Felix Zielcke
Post by Felix Zielcke
Oh well I should have used grep with -i
meta_bg and META_BG does make a difference
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.
Felix Zielcke
2008-07-16 19:33:07 UTC
Permalink
From: "JavierMartín" <***@gmail.com>
Sent: Wednesday, July 16, 2008 9:07 PM
To: "The development of GRUB 2" <grub-***@gnu.org>
Subject: Re: grub-probe detects ext4 wronly as ext2
Post by Javier Martín
OK, so this is what I get from your 3 posts, and my proposals for the
* 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
Post by Javier Martín
* 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
Post by Javier Martín
* 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
Robert Millan
2008-07-19 14:27:07 UTC
Permalink
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
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().
Post by Javier Martín
- 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 /.)
Javier Martín
2008-08-11 14:14:00 UTC
Permalink
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
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
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.
Post by Robert Millan
Post by Javier Martín
- 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
Post by Robert Millan
--
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
http://lists.gnu.org/mailman/listinfo/grub-devel
Felix Zielcke
2008-08-27 13:58:28 UTC
Permalink
Hello list,
Post by 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
Robert Millan
2008-08-30 11:17:53 UTC
Permalink
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Javier Martín
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.
Post by Javier Martín
- if (grub_errno)
+ if (grub_errno != GRUB_ERR_NONE)
Why?
Post by Javier Martín
- 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.
Post by Javier Martín
+ /* 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."
Javier Martín
2008-08-30 21:28:25 UTC
Permalink
Hello there! It's nice to be in town again, though post-vacation
syndrome is hitting me full... Resuming the thread,
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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.
Post by Robert Millan
Post by Javier Martín
- 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.
Post by Robert Millan
Post by Javier Martín
- 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.
Post by Robert Millan
Post by Javier Martín
+ /* 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
Javier Martín
2008-09-24 17:05:33 UTC
Permalink
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.
Post by Javier Martín
Hello there! It's nice to be in town again, though post-vacation
syndrome is hitting me full... Resuming the thread,
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
Post by Javier Martín
Post by Robert Millan
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.
Post by Robert Millan
Post by Javier Martín
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.
Post by Robert Millan
Post by Javier Martín
- 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.
Post by Robert Millan
Post by Javier Martín
- 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
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.
Post by Robert Millan
Post by Javier Martín
+ /* 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
Felix Zielcke
2009-02-04 07:41:05 UTC
Permalink
Post by 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
Javier Martín
2009-02-04 13:08:58 UTC
Permalink
Post by Felix Zielcke
Post by 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.
Felix Zielcke
2009-02-07 19:30:14 UTC
Permalink
Post by Javier Martín
Well, I am happy to post a diff of the patch against current SVN head
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
Javier Martín
2009-02-07 23:54:29 UTC
Permalink
Post by Felix Zielcke
Post by Javier Martín
Well, I am happy to post a diff of the patch against current SVN head
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" ;)
Robert Millan
2009-02-08 00:28:20 UTC
Permalink
Post by Javier Martín
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.
Post by Javier Martín
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."
Robert Millan
2008-07-01 16:03:35 UTC
Permalink
Post by Javier Martín
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 /.)
Loading...