Re: CVS commit: src/sys/miscfs/kernfs

[christos%zoulas.com@localhost (Christos Zoulas)]

On Jun 23,  9:07pm, bouyer%antioche.eu.org@localhost (Manuel Bouyer) wrote:
-- Subject: Re: CVS commit: src/sys/miscfs/kernfs

| On Fri, Jun 23, 2006 at 02:54:12PM -0400, Christos Zoulas wrote:
| > On Jun 23,  8:16pm, bouyer%antioche.eu.org@localhost (Manuel Bouyer) wrote:
| > -- Subject: Re: CVS commit: src/sys/miscfs/kernfs
| > 
| > | > 1. If you don't kernfs_alloctype (which obviously xen does so it works)
| > | >    the splay tree will be empty so the read method will not be found.
| > | 
| > | Yes, that's what I found. All the ops using kernfs_try_fileop() are 
broken.
| > | I can confirm that write are broken too, and the write code has been there
| > | for years.
| > 
| > I also don't understand the assignment in the first loop:
| > 
| >     dkf[i].kf_type = nextfreetype;
| 
| This is kernfs_alloctype(). Each time we allocate a new kernfs type,
| a new fileop structure is added to the splay tree for this type.

So all the built-in types have the same kf_type? 

        for (i = 0; i < sizeof(kernfs_default_fileops) /
                     sizeof(kernfs_default_fileops[0]); i++) {
                dkf[i].kf_type = nextfreetype;
                SPLAY_INSERT(kfsfileoptree, &kfsfileoptree, &dkf[i]);
        }

Nextfreetype did not get incremented here.
And here we could move the assignment out of the loop?

        for (i = 0; i < nkf; i++) {          
                skf.kf_type = nextfreetype;
                skf.kf_fileop = kf[i].kf_fileop;
                if ((fkf = SPLAY_FIND(kfsfileoptree, &kfsfileoptree, &skf)))
                        fkf->kf_genop = kf[i].kf_genop;
        }

What about if we did not find any that matched? Do we still increment
nextfreetype?
        
        return nextfreetype++;

| > | Agreed for read(). write in this state for years, so I don't know if
| > | write silently failing was intended or not.
| > 
| > I think it is an accident. In fact, I think that the whole splay tree
| > code is overkill; we have ~ 10 fileops and for that a sorted array is
| > good enough. Or even a non-sorted one!
| 
| One array per (user-defined) type, so it's a dual-dimenention array.
| It would be enouth, the issue is that it's dynamically created when
| user-defined types are registered.

Ok, now I get it.

| > | This code has been there for more than 3 months. Either kernfs is not
| > | used much outside of Xen and leaving it in this state a few more days 
while
| > | a real fix is found is not a big deal, it broke recently and my code is 
| > | not to blame. 
| > 
| > Well, most of the nodes are not writable, and you broke the read path.
| > 
| > | Anyway I'm looking at it.
| > 
| > I am fine with that. I don't agree though that you put back the broken
| > code till you found a fix. Unless you believe that the # of Xen users
| > is the majority.
| 
| All Xen users are affected by the backout, and i386 users too because this
| break release builds for the i386 port. Non-Xen users probably don't care
| much as this has been brocken for more than 3 months and nobody noticed
| until today.