Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

On Fri, Jul 04, 2008 at 04:45:02PM +0200, Javier Martín wrote:
> 
> By the way, I'm already using SVN (and thus svn diff) for this patch. Is
> that right? Was the migration completed already?

Yep.

> +/* Superblock filesystem feature flags (RW compatible) */
> +#define EXT2_FEATURE_COMPAT_DIR_PREALLOC     0x0001
> +#define EXT2_FEATURE_COMPAT_IMAGIC_INODES    0x0002
> +#define EXT3_FEATURE_COMPAT_HAS_JOURNAL              0x0004
> +#define EXT2_FEATURE_COMPAT_EXT_ATTR         0x0008
> +#define EXT2_FEATURE_COMPAT_RESIZE_INODE     0x0010
> +#define EXT2_FEATURE_COMPAT_DIR_INDEX                0x0020
> +/* Superblock filesystem feature flags (RO compatible) */
> +#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER  0x0001
> +#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE    0x0002
> +#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR     0x0004
> +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM              0x0010
> +#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK     0x0020
> +#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE   0x0040
> +/* Superblock filesystem feature flags (back-incompatible) */
> +#define EXT2_FEATURE_INCOMPAT_COMPRESSION    0x0001
> +#define EXT2_FEATURE_INCOMPAT_FILETYPE               0x0002
> +#define EXT3_FEATURE_INCOMPAT_RECOVER                0x0004 /* Needs 
> recovery */
> +#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV    0x0008 /* Journal device */
> +#define EXT2_FEATURE_INCOMPAT_META_BG                0x0010
> +#define EXT4_FEATURE_INCOMPAT_EXTENTS                0x0040 /* extents 
> support */
> +#define EXT4_FEATURE_INCOMPAT_64BIT          0x0080
> +#define EXT4_FEATURE_INCOMPAT_FLEX_BG                0x0200

I'd suggest making the "RW compatible" etc notes a bit more ellaborate to make
it clear what they mean (I'm confused myself).
 
> +/* The set of back-incompatible features this driver DOES support. Add (OR)
> + * flags here as the related features are implemented into the driver */
> +#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE )

I suppose we'll want to have EXT4_FEATURE_INCOMPAT_EXTENTS here, now that it's
been implemented (Bean just sent a patch, which will probably be merged first).

> +/* The set of back-incompatible features this driver DOES NOT support but are
> + * ignored for some hackish reason. Flags here should be here _temporarily_!
> + * Remember that INCOMPAT_* features are so for a reason! */
> +#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )

Instead of this can we have an explanation of what EXT3_FEATURE_INCOMPAT_RECOVER
is doing here?  I think the reason was that our code for this feature wasn't
as mature as it should be, and it appears that not handling it brings better
results in the short term.

Maybe Pavel can ellaborate more, I think it was him who brought up this point.

> +  /* Check the FS doesn't have feature bits enabled that we don't support. */
> +  if (grub_le_to_cpu32 (data->sblock.feature_incompat)
> +        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
> +    goto fail;
> [...]
>   fail:
> -  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
> +  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem, or incompatible"
> +                               "features enabled");

Since we know which one applies, why not tell grub_error about it?  We could
leave the "not an ext2 filesystem" call unmodified and add another one for
this particular error.

-- 
Robert Millan

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