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 /.) _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel