Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

El sáb, 05-07-2008 a las 14:07 +0200, Robert Millan escribió:
> However, adding new strings is expensive, since they tend to take size more
> easily than code.  I would be careful about that.
I checked the space requirements, and seemingly there was a bit of space
available in the .rodata zone, since all those strings only added less
than 20 bytes o_O (at least in i386-pc). Wrapping it up, the pre-post
delta including code and strings is 120 bytes in i386-pc.

> >  grub_ext2_mount (grub_disk_t disk)
> >  {
> >    struct grub_ext2_data *data;
> > +  const char *err_msg = 0;
> 
> Is this "const" right?  You're modifiing its value.
Yeah, err_msg is a "pointer to (const char)", thus the characters are
unmodifiable but the pointer can be reassigned. You can also have "char
* const", which is a "const pointer to char" (I don't see much utility
to it); and finally "const char * const", a "const pointer to (const
char)", which would be the constant, unreassignable string pointer.

AFAIK, since GCC 4.0 string literals are "const char *" by default and
are stored in .rodata, so if the variable was termed as just "char *"
we'd have needed a cast. However, if the compiler considers string
literals as "char *", no cast is needed to make it "const char *".

> >    grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
> >                    (char *) &data->sblock);
> >    if (grub_errno)
> > -    goto fail;
> > +    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
> 
> This overrides the grub_errno and grub_errmsg provided by grub_disk_read and
> replaces them with values that hide the true problem.  If there was a disk
> read error, we really want to know about it from the lower layer.
Well, the old version did just the same (even worse, because the message
was generic). What would be the correct path of action here? I mean, how
can we propagate the error messages?

>  
> >    /* Make sure this is an ext2 filesystem.  */
> >    if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
> > -    goto fail;
> > +    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem (superblock magic 
> > mismatch)")
> 
> No need to ellaborate here;  by definition, the magic number makes the
> difference between a corrupt ext2 and something that is not ext2.  So
> you can just say it's not ext2.
Ok, I'm sending a new version of the thing with that part removed. I'm
trying to think of a ChangeLog entry for this. What about this?
        fs/ext2.c: extN driver will reject filesystems with unimplemented
"incompatible" features enabled in the superblock

ext2_incompat.patch.4
Description: Text Data

signature.asc
Description: Esta parte del mensaje está firmada digitalmente