Date: Wed, 14 Aug 2013 19:10:51 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, "Pedro F. Giffuni" <pfg@FreeBSD.org>, src-committers@FreeBSD.org Subject: Re: svn commit: r254286 - head/sys/fs/ext2fs Message-ID: <20130814182851.A1065@besplex.bde.org> In-Reply-To: <9B5BBD34-F953-40BF-8C10-0EF466ED3350@FreeBSD.org> References: <201308131839.r7DIdaLD037277@svn.freebsd.org> <9B5BBD34-F953-40BF-8C10-0EF466ED3350@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 14 Aug 2013, Dimitry Andric wrote:
> On Aug 13, 2013, at 20:39, Pedro F. Giffuni <pfg@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130814182851.A1065>