Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

Hello there! It's nice to be in town again, though post-vacation
syndrome is hitting me full... Resuming the thread,

El sáb, 30-08-2008 a las 13:17 +0200, Robert Millan escribió:
> 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.
Ok, so that's already done in the previous patch.

> 
> >  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.
I don't object at all, but 0 is used throughout the GRUB code to
represent null pointers (see the args struct in any command source
file), so I don't understand the criterion being applied here.

> 
> > -  if (grub_errno)
> > +  if (grub_errno != GRUB_ERR_NONE)
> 
> Why?
1st, because the condition being checked is explicit and thus clearer.
These int->bool C conventions are, even though enshrined by ANSI and
thus pretty standard and reliable, irky at best and due only to the lack
of a proper boolean type. As I think I stated before, the only cases I
use such "cast" is when checking for null pointers and when using
variables that are explicitly boolean in nature.
2nd, because the new code does not assume that GRUB_ERR_NONE is defined
to zero. Even though this definition will most likely never change, we
might one day decide that -42 is a better value for success.
3rd, because in addition to all that, with any sensible compiler, the
additional binary size cost is nothing at all.

> 
> > -    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.
It is not meant to provide any runtime improvement, it's just for
consistency: since local_error is already zero, a "goto fail" would
suffice but I think this uniformity adds readability, not hinders it:
the referenced macro is at the very top of the function, not on a
included header, so the reference is not very time-consuming; and it's
not very complex, so it only needs to be read once. Besides, most
compilers should just optimize the extra assignment away given that the
value of local_error is ct-known for the code paths leading to that
statement and it is not a "volatile" variable.

> 
> > +  /* 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.
OK, removed the comment. Maybe a similar comment will be fine over the
macro, though.

That was all, folks! Given that I (think I) addressed your two main
objections, I won't send a new patch right now. I will continue to read
the list this month, but my availability is likely to vary wildly, as I
will be trapped by the ensnaring bureaucracy of the Spanish
universities, scholarships, etc.

-Habbit

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