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