Re: fusefs-kmod broken?

Kostik Belousov wrote:
> 
> --7hK5U8dVDlZxii7z
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Mon, Aug 23, 2010 at 03:47:23PM +0200, Ed Schouten wrote:
> > * Kostik Belousov <kostikbel_at_gmail.com> wrote:
> > > On Mon, Aug 23, 2010 at 03:35:55PM +0200, Ed Schouten wrote:
> > > > * Kostik Belousov <kostikbel_at_gmail.com> wrote:
> > > > > Which most likely means that fusesfs filled its own struct fileops
> > > > > without properly initializing fo_truncate member.
> > > >=20
> > > > It's a bit misleading that cdevs automatically patch the table, while
> > > > the fileops don't. Maybe it would be a good idea to patch finit() to
> > > I do not understand your first sentence. Would you please elaborate ?
> >=20
> > Say, you create a cdev, if you don't implement all ops, it will check
> > for null pointers and return error codes accordingly. This doesn't
> > happen for fileops, which is probably one of the reasons why people
> > sometimes forget to implement them.
> >=20
> > Wouldn't it be better to prevent this form of footshooting by adding
> > assertions? This will add some overhead for any file descriptor created,
> > but a kernel with INVARIANTS isn't meant to be fast.
> Thanks, I see it now.
> 
> The cdev interface definitely falls into the public kernel interface.
> Having to fill all cdevsw methods for a random driver is too much
> burden put on the several dozens maintainers.
> 
> On the other hand, file level is not much widely used by third-party
> components, and even in-tree code implements only ten different file
> types.
> 
> I would not object loudly if someone put such checks as proposed
> under INVARIANTS, but also I do not see a real point in having them.
> Might be slightly better to put the checks, again under INVARIANTS,
> in the fo_XXX() wrappers.

So, in this case is the fusefs module broken?  I'm guessing it is.
I don't like the way fuse_fileops is initialised in fuse4bsd.  I
would prefer for the struct to be zeroed and then the fo_xxx
implimented bits set as appropriate.  That way when the struct is
changed, you don't get stung again.

Patch attached to that makes fusefs-kmod not blowup kernels post this change.

Ian

-- 
Ian Freislich

Index: files/patch-fuse_module__fuse_vnops.c
===================================================================
RCS file: /home/ncvs/ports/sysutils/fusefs-kmod/files/patch-fuse_module__fuse_vnops.c,v
retrieving revision 1.4
diff -u -d -r1.4 patch-fuse_module__fuse_vnops.c
--- files/patch-fuse_module__fuse_vnops.c	30 Oct 2008 15:36:35 -0000	1.4
+++ files/patch-fuse_module__fuse_vnops.c	23 Aug 2010 14:27:17 -0000
+_at__at_ -214,6 +214,7 _at__at_
+          * following fields are filled from vnops, but "vnops.foo" is not
+          * legitimate in a definition, so we set them at module load time
+ 	 */
++	.fo_truncate = NULL,
+ 	.fo_ioctl    = NULL,
+ 	.fo_poll     = NULL,
+ 	.fo_kqfilter = NULL,
+_at__at_ -799,8 +800,11 _at__at_
  	struct vnode *vp = ap->a_vp;
  	struct vattr *vap = ap->a_vap;
  	struct ucred *cred = ap->a_cred;
_at__at_ -13,7 +21,7 _at__at_
  	struct fuse_dispatcher fdi;
  	struct timespec uptsp;
  	int err = 0;
-_at__at_ -871,7 +874,11 _at__at_
+_at__at_ -871,7 +875,11 _at__at_
  fuse_access(ap)
  	struct vop_access_args /* {
  		struct vnode *a_vp;
_at__at_ -25,7 +33,7 _at__at_
  		struct ucred *a_cred;
  		struct thread *a_td;
  	} */ *ap;
-_at__at_ -886,7 +893,13 _at__at_
+_at__at_ -886,7 +894,13 _at__at_
  	else
  		facp.facc_flags |= FACCESS_DO_ACCESS;
  
_at__at_ -40,7 +48,7 _at__at_
  }
  
  /*
-_at__at_ -946,7 +959,11 _at__at_
+_at__at_ -946,7 +960,11 _at__at_
  		/* We are to do the check in-kernel */
  
  		if (! (facp->facc_flags & FACCESS_VA_VALID)) {
_at__at_ -53,7 +61,7 _at__at_
  			if (err)
  				return (err);
  			facp->facc_flags |= FACCESS_VA_VALID;
-_at__at_ -1929,7 +1946,11 _at__at_
+_at__at_ -1929,7 +1947,11 _at__at_
  		 * It will not invalidate pages which are dirty, locked, under
  		 * writeback or mapped into pagetables.") 
  		 */
_at__at_ -65,7 +73,7 _at__at_
  		fufh->flags |= FOPEN_KEEP_CACHE;
  	}
  
-_at__at_ -3005,8 +3026,11 _at__at_
+_at__at_ -3005,8 +3027,11 _at__at_
  	struct vattr *vap = ap->a_vap;
  	struct vnode *vp = ap->a_vp;
  	struct ucred *cred = ap->a_cred;