Re: grub-probe detects ext4 wronly as ext2

[Robert Millan <rmh@aybabtu.com>]

On Mon, Aug 11, 2008 at 04:14:00PM +0200, Javier Martín wrote:
> >> > 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.

>  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.

> -  if (grub_errno)
> +  if (grub_errno != GRUB_ERR_NONE)

Why?

> -    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.

> +  /* 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."