Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacingrq_for_each_bio

[Geert Uytterhoeven]

On Sat, 18 Aug 2007, Satyam Sharma wrote:
> On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > >> [...]
> > >> >  		dev_dbg(&dev->sbd.core,
> > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > >> > -			bio_sectors(bio), sector);
> > >> > -		bio_for_each_segment(bvec, bio, j) {
> > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > >> > +			bio_sectors(iter.bio),
> > >> > +			(unsigned long)iter.bio->bi_sector);
> > >>                         ^^^^^^^^^^^^^^^
> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > 
> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > so since sector_t may just change its shape underneath. It would not be so much
> > of a problem if printf() was not a varargs function, but it is, and hence,
> > passing an object bigger than the format specifier can make problems.
> 
> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> I was mentioning why the cast itself should be (unsigned long long) otoh.

On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
no compiler warning to be silenced by adding the cast.

On the other hand, adding a cast may hide bugs:
  - cast to unsigned long: When reusing parts of this code for a new 32-bit
    driver, the sector_t will be silently truncated,
  - cast to unsigned long long: When sector_t is changed to an even larger
    type, it will be silently truncated as well.
Without the cast, these would cause compiler warnings.

BTW, the resulting code didn't even compile, because of 3 bugs:

| linux-2.6/drivers/block/ps3disk.c: In function âps3disk_scatter_gatherâ:
| linux-2.6/drivers/block/ps3disk.c:115: error: âbioâ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:115: error: (Each undeclared identifier is reported only once
| linux-2.6/drivers/block/ps3disk.c:115: error: for each function it appears in.)
| linux-2.6/drivers/block/ps3disk.c:115: error: âjâ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:116: error: implicit declaration of function âbio_kunmap_bvecâ
| make[3]: *** [drivers/block/ps3disk.o] Error 1

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village  Da Vincilaan 7-D1  B-1935 Zaventem  Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@xxxxxxxxxxx	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7  B-1840 Londerzeel  Belgium	
VAT BE 0413.825.160 Â RPR Brussels	
Fortis Bank Zaventem  Swift GEBABEBB08A  IBAN BE39001382358619