From owner-svn-src-all@FreeBSD.ORG Wed Aug 14 09:31:45 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 80DB7C8D; Wed, 14 Aug 2013 09:31:45 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 111CE2B0C; Wed, 14 Aug 2013 09:31:44 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id E42737812C4; Wed, 14 Aug 2013 19:10:53 +1000 (EST) Date: Wed, 14 Aug 2013 19:10:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric Subject: Re: svn commit: r254286 - head/sys/fs/ext2fs In-Reply-To: <9B5BBD34-F953-40BF-8C10-0EF466ED3350@FreeBSD.org> Message-ID: <20130814182851.A1065@besplex.bde.org> References: <201308131839.r7DIdaLD037277@svn.freebsd.org> <9B5BBD34-F953-40BF-8C10-0EF466ED3350@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=YYGEuWhf c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=Eygb-0caP3YA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=AD96D3NyImQA:10 a=6I5d2MoRAAAA:8 a=JCPPM7_Sc5d9gZvzo44A:9 a=AD96TLpiGArUWDxL:21 a=9KybAjeJlIn2ttJd:21 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, "Pedro F. Giffuni" , src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Aug 2013 09:31:45 -0000 On Wed, 14 Aug 2013, Dimitry Andric wrote: > On Aug 13, 2013, at 20:39, Pedro F. Giffuni wrote: >> Log: >> ext2fs: update format specifiers for ext4 type. This is still quite broken. >> Modified: head/sys/fs/ext2fs/ext2_subr.c >> ============================================================================== >> --- head/sys/fs/ext2fs/ext2_subr.c Tue Aug 13 18:14:53 2013 (r254285) >> +++ head/sys/fs/ext2fs/ext2_subr.c Tue Aug 13 18:39:36 2013 (r254286) >> @@ -150,7 +150,7 @@ ext2_checkoverlap(struct buf *bp, struct >> ep->b_blkno + btodb(ep->b_bcount) <= start) >> continue; >> vprint("Disk overlap", vp); >> - (void)printf("\tstart %d, end %d overlap start %lld, end %ld\n", >> + (void)printf("\tstart %ld, end %ld overlap start %lld, end %ld\n", >> start, last, (long long)ep->b_blkno, >> (long)(ep->b_blkno + btodb(ep->b_bcount) - 1)); >> panic("Disk buffer overlap"); > > > This still fails on arches where int64_t is aliased to long long > (basically, the 32-bit arches). Since using PRId64 is apparently > frowned upon, the easiest solution is to cast the 'start' and 'last' > variables to long long, and print them using %lld. Gak. Later you said that this is to match the surrounding style. But the surrounding style is bad, and has lots of type errors that become bugs after changes like the recent ones: - (void)'ing printf() is a style bug - the above change breaks the lexical style by blindly expanding the line beyond 80 columns. (void)ing printf() helps give such style bugs by wastin6 6 columns - the continuation indent for the printf() is 8 columns, unlike the KNF continuation indent of 5 columns which is used a few lines earlier - it is nonsense to cast the overlap-starting block numer to a wider type than the overlap-ending block number, since the latter is at least as large. At least after recent changes, the cast to long became a type error on arches with 32-bit longs, since the block number type is now wider. I think it is now 64 bits. I disapprove of any changes to make block number types unsigned. If the block number type was actually changed to uint32_t for ext2, then printing it it using long is still a type error on arches with 32-bit longs. The cast to long is bogus and mainly breaks compile-time detection of the error. - the long long abomination is used. The cast to long long is bogus, but doesn't hide any error provided the block number type remains at most 64 bits signed. - since (apparently) no printf error is detected for the non-overlap start and end, these variables must have type long to match their printf format. But they actually have type e4fs_daddr_r, which is int64_t, at least now. int64_t happens to be the same as long on 64-bit arches. On 32-bit arches, it is very far from matches. So the non-detection of printf format errors from this just shows null testing on 32-bit arches. This function is a sanity check under KDB. Hopefully it is more insane than what it checks. Until recently it used int32_t for start and end. The hard-coded printf format of %d for them only assumed 32-bit ints. The bogus cast to long was defense against an old printf format error in one of the args (the overlap-ending block number) in 2000. Versions before that were broken on 64-bit arches. The bogus cast to long long was defense in 2002 against the expansion of the system-wide block number type daddr_t a little earlier. A previous buggy change changed the out of data format %d to %lld. %d matched daddr_t == int32_t accidentally on all arches. %lld matched daddr_t == int64_t accidentally only on 32-bit arches. It shouldn't take 4 commits per arg to get printf formats right. Bruce