Date: Thu, 25 Nov 2010 16:32:13 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Maste <emaste@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r215811 - head/sys/boot/common Message-ID: <20101125155736.G1888@besplex.bde.org> In-Reply-To: <201011250316.oAP3GVvK092173@svn.freebsd.org> References: <201011250316.oAP3GVvK092173@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101125155736.G1888>