svn commit: r215811 - head/sys/boot/common

[Bruce Evans]

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