On Thu, 25 Nov 2010, Ed Maste wrote:
Log:
Give a bit of a hint of the failure (read != expected) but don't make
the error message needlessly more verbose.
Discussed with: attilio
Any chance of not making the source code needlessly verbose and full of
style and type bugs?
Modified: head/sys/boot/common/load_elf.c
==============================================================================
--- head/sys/boot/common/load_elf.c Thu Nov 25 03:02:53 2010
(r215810)
+++ head/sys/boot/common/load_elf.c Thu Nov 25 03:16:31 2010
(r215811)
@@ -453,7 +453,7 @@ __elfN(loadimage)(struct preloaded_file
}
result = archsw.arch_readin(ef->fd, lastaddr, shdr[i].sh_size);
if (result < 0 || (size_t)result != shdr[i].sh_size) {
- printf("\nelf" __XSTRING(__ELF_WORD_SIZE) "_loadimage: could not read
symbols - skipped! (%ju - %ju)", (uintmax_t)result,
+ printf("\nelf" __XSTRING(__ELF_WORD_SIZE) "_loadimage: could not read
symbols - skipped! (%ju != %ju)", (uintmax_t)result,
(uintmax_t)shdr[i].sh_size);
lastaddr = ssym;
ssym = 0;
This code is obviously not concerned about space or time optimizations,
else it wouldn't use uintmax_t, but it uses __XSTRING(__ELF_WORD_SIZE)
to convert an integer to a string at compile time. This makes it more
verbose and helps give it a style bug (a too-long line). Recent commits
expanded the style bug by lengthening the line to print another arg,
despite the careful line splitting for the other arg.
The cast to size_t at the start of this code is bogus. It assumes that the
type of sh_size is no smaller than that of size_t, but if you assume that
then you can assume it in the printf too and cast everything to size_t (*).
This assumption may be valid, but elf itself uses careful type definitions
(not involving size_t) to avoid such assumptions. Assuming this in the
diagnostic printf is less risky than assuming it in the error checking.
(*) Casting `result' to either uintmax_t or size_t in the printf is wrong,
since `result' is a signed type and one of the error cases reported by
this diagnostic is when result < 0. `result' actually has type ssize_t,
and it can be -1 after a read error. ssize_t is somewhat inconsistent
with typeof(sh_size), but good enough. Variables of type ssize_t should
by printed using %zd and not mispromoted to uintmax_t for printing with
%ju. I think this code uses libstand printf, which supports %zd.
These and other fixes fixes give something like:
if (result < 0 || (Elf_mumble))result != shdr[i].sh_size) {
printf(
"\nelf%d_loadimage: could not read symbols (%zd != %ju) -- skipped",
__ELF_WORD_SIZE, result, (uintmax_t)shdr[i].sh_size);
other fixes:
- I couldn't find anything good for Elf_mumble. Elf declarations seem
to actively inhibit declaring the types of things in a size-independent
way. sh_size is declared as type Elf32_Word or ELf64_Xword.
- the strange leading newline with no trailing newline is preserved
- the string is still long so it needs outdenting to fit
- rephrase message to put the error info before the action.
- fix rendering of the dash symbol
- remove shouting (!). More rephrasing or different termination may be
needed if the string is expanded, as encouraged by its not having a
trailing newline.
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
