From owner-svn-src-head@FreeBSD.ORG Thu Nov 25 05:32:19 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E55A41065672; Thu, 25 Nov 2010 05:32:18 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id 6DD248FC17; Thu, 25 Nov 2010 05:32:18 +0000 (UTC) Received: from c122-106-145-124.carlnfd1.nsw.optusnet.com.au (c122-106-145-124.carlnfd1.nsw.optusnet.com.au [122.106.145.124]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id oAP5WD7I015003 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 25 Nov 2010 16:32:14 +1100 Date: Thu, 25 Nov 2010 16:32:13 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ed Maste In-Reply-To: <201011250316.oAP3GVvK092173@svn.freebsd.org> Message-ID: <20101125155736.G1888@besplex.bde.org> References: <201011250316.oAP3GVvK092173@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r215811 - head/sys/boot/common X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Nov 2010 05:32:19 -0000 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