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

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

dholland@ wrote:

> 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.

If we claim it's wrong, we should have properly documented
API definitions that describe what's correct.

We can also define d_secsize=0 is valid and it means attached
but uninitialized devices, to simplify driver's implementation.

>  > > > 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.

There is no easy way to confirm all the drivers are fixed.
md(4) was fixed, but who will check other drivers?

The problem is getdisksize() was introduced after 5.0
and it assumed DIOCGPART always returned valid values,
but no one confirmed if the assumption was correct.

In this case, I have a feeling that it's simpler and
reasonable to add sanity checks in getdisksize(),
unless someone[TM] will check all drivers.

(though I notice ffs_wapbl.c already has KDASSERT())
---
Izumi Tsutsui