Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

Well, seeing that Robert is back (and what a torrent of activity!), I'll
shamelessly try to resubmit my last post from nearly a month ago in the
hope that he'll notice it.

El sáb, 30-08-2008 a las 23:28 +0200, Javier Martín escribió:
> 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