Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

On Fri, Jul 04, 2008 at 10:41:35PM +0200, Javier Martín wrote:
> Wonderful! I was sick of jumping through hoops with cvs diff.

I wasn't even using cvs diff!  (you don't want to know what my replacement
dance was)  ;-)

> > I'd suggest making the "RW compatible" etc notes a bit more ellaborate to 
> > make
> > it clear what they mean (I'm confused myself).
> Done, though now I might have over-elaborated

I think that's ok, comments don't eat space, so it's better to risk explaining
too much than being short of explaining everything.

> > Since we know which one applies, why not tell grub_error about it?  We could
> > leave the "not an ext2 filesystem" call unmodified and add another one for
> > this particular error.
> > 
> I may have overstepped a bit, but I've thought it more sensible to
> replace all "goto fail;"s for calls to a new macro MOUNT_FAIL taking a
> string argument which is saved in the new variable err_msg, and then
> jumps to fail which shows _that_ message instead of the old one. Then, I
> wrote informative messages for each error condition instead of just "not
> an ext2 filesystem".

Looks a bit ugly, but I don't have any objection if it makes code smaller
(by eliminating duped grub_error calls).

However, adding new strings is expensive, since they tend to take size more
easily than code.  I would be careful about that.

>  grub_ext2_mount (grub_disk_t disk)
>  {
>    struct grub_ext2_data *data;
> +  const char *err_msg = 0;

Is this "const" right?  You're modifiing its value.
 
>    grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
>                    (char *) &data->sblock);
>    if (grub_errno)
> -    goto fail;
> +    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")

This overrides the grub_errno and grub_errmsg provided by grub_disk_read and
replaces them with values that hide the true problem.  If there was a disk
read error, we really want to know about it from the lower layer.
 
>    /* Make sure this is an ext2 filesystem.  */
>    if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
> -    goto fail;
> +    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem (superblock magic 
> mismatch)")

No need to ellaborate here;  by definition, the magic number makes the
difference between a corrupt ext2 and something that is not ext2.  So
you can just say it's not ext2.

> +  /* Check the FS doesn't have feature bits enabled that we don't support. */
> +  if (grub_le_to_cpu32 (data->sblock.feature_incompat)
> +        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
> +    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible 
> features")

Ok.

>    if (grub_errno)
> -    goto fail;
> +    EXT2_DRIVER_MOUNT_FAIL("could not read the root directory inode")

Ok.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)