Re: buf->b_blkno v.s. buf->b_secno (Re: CVS commit: src/sys (moving to tech-kern))

[David Young <dyoung%pobox.com@localhost>]

On Thu, Jan 08, 2009 at 01:57:13PM +0100, Reinoud Zandijk wrote:
> Hi Andrew and Matthew,
> 
> On Wed, Jan 07, 2009 at 09:05:54PM +0000, Andrew Doran wrote:
> > On Sun, Jan 04, 2009 at 06:33:50PM +1100, matthew green wrote:
> > > !       /*
> > > !        * XXX somehow memory is getting corrupted on 2048 byte sectors 
> > > if its
> > > !        * just 2 times 2048!! It even reads only 2048 bytes max in one 
> > > go on
> > > !        * optical media.
> > > !        */
> > > !
> > > !       a.bp = geteblk(3 * (int)lp->d_secsize);
> 
> Found the critter... its the ancient `buf->b_blkno' needs to be in DEV_BSIZE.
> disk_read_sectors() has as argument a sectornr and a count... but the
> sectornr needs to be in DEV_BSIZE units and count needs to be in sectorsize
> units (!!)
> 
> I could easily fixup the disk_read_sectors() call to make it do what it claims
> to do: read a disk sector since its only used in 3 places.
> 
> Another bigger one would be to move buf->b_blkno to allways specify sectorsize
> units. This would at least mean conversion of scsipi/cd scsipi/sd (if needed)
> and the fixups in iso9660 and UDF to follow this.

Maybe one can annotate struct members and the arguments in function
declarations with their units, and then use a tool such as Sparse
to make sure that units are not mismatched in the kernel.  I'm
thinking of annotations looking like

int __units(sector) b_blkno;

One could re-check the units if one changes them.
 
Just an idea.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933