svn commit: r254286 - head/sys/fs/ext2fs

[Bruce Evans]

On Wed, 14 Aug 2013, Dimitry Andric wrote:

> On Aug 13, 2013, at 20:39, Pedro F. Giffuni <pfg at 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