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