On Fri, Jun 23, 2006 at 03:31:06PM -0400, Christos Zoulas wrote:
> On Jun 23,  9:07pm, bouyer@antioche.eu.org (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 (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? 

No, but they are all below KFSlasttype (nextfreetype is statically
initialized with KFSlasttype)

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

No, it's incremented later. All the fileops are for the same type.

> 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;
>         }

We could but it's not as easy: for each element in kf[] we'll have to find
the corresponding one in dkf[]. The SPLAY_FIND() here does it for us.

> 
> What about if we did not find any that matched? Do we still increment
> nextfreetype?

Yes, we still added an entry in the splay, this type is allocated and has
kernfs VOPS

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--