Re: CVS commit: src/sys/fs/msdosfs

[David Holland <dholland-sourcechanges%netbsd.org@localhost>]

On Wed, Jul 04, 2012 at 01:45:08AM +0900, Izumi Tsutsui wrote:
 > > getdisksize() is not complicated; it tries DIOCGPART and then
 > > DIOCGWEDGEINFO and that's about it.
 > 
 > Yes, but it is not implementation issue but design issue.
 > 
 > > I think it's wrong for either of those to be returning a size of 0.
 > 
 > Yes, but which one? getdisksize() or DIOCGPART in each driver?

There's nothing getdisksize can do besides return what the driver
says. If the driver is reporting nonsense, it's wrong.

 > > > I can change it to KASSERT if we can assume that partinfo taken
 > > > from DIOCGPART must be initialized properly on all disk(9) devices,
 > > > i.e. if we can say disklab->d_secsize==0 means driver's bug,
 > > > but I could not find any evidence.
 > > >
 > > > All vfs mountfs functions are called in case of "root on generic"
 > > > via mountroot in setroot() so I thought returning error was better
 > > > than panic in that case, even after md(4) was fixed.
 > > > But if you don't agree it, please revert it.
 > > 
 > > If we know it can happen, crashing isn't very productive.
 > > 
 > > However, can you move the test to inside getdisksize()? Make it print
 > > a message and return EINVAL (or something). That way things other than
 > > msdosfs are also protected.
 > 
 > It depends how the design issue is resolved.
 > If DIOCGPART must not return secsize=0,
 > KASSERT in getdisksize() seems reasonable.
 > If uninitialized secsize=0 from DIOCGPART is acceptable,
 > getdisksize() or each caller should have sanity checks.

Sure, all I'm saying is that since we know it can happen, printing a
message and returning an error (and not just when DIAGNOSTIC) is
probably better than crashing. Once we think the drivers are fixed,
then it can become an assertion.

-- 
David A. Holland
dholland%netbsd.org@localhost