Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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

Home | Main Index | Thread Index | Old Index