Untangle proc * in VOP_*

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Untangle proc * in VOP_*

Martin Pieuchot
Most of the VOP_* methods include an argument of type "struct proc *"
called `a_p'.  This pointer is always set to `curproc' as confirmed by
the diff below.  The diff has been though base build with NFS on amd64
and sparc64 as well as a full port bulk on amd64 and is in the current
octeon port bulk.

I'd like to commit it in order to start removing the requirement of
passing a "struct proc *" pointer which is always set to curproc.  This
is unnecessary and has spread in many other places in the kernel.

That makes codes unnecessarily complicated:
 - a "struct proc *" is carried over just to satisfy already complex
   interfaces
 - it isn't obvious which "struct proc *" is not the curproc and
   asserts like KASSERT(p == curproc) are being made
 - in many places where curproc is used people put XXX as if it was
   wrong

So this is just a starting diff, cleanups can start afterward.

Ok?

Index: kern/vfs_vops.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_vops.c,v
retrieving revision 1.25
diff -u -p -r1.25 vfs_vops.c
--- kern/vfs_vops.c 14 Feb 2020 11:57:56 -0000 1.25
+++ kern/vfs_vops.c 16 Mar 2020 14:16:42 -0000
@@ -145,6 +145,8 @@ VOP_OPEN(struct vnode *vp, int mode, str
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
+
  if (vp->v_op->vop_open == NULL)
  return (EOPNOTSUPP);
 
@@ -164,6 +166,7 @@ VOP_CLOSE(struct vnode *vp, int fflag, s
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  ASSERT_VP_ISLOCKED(vp);
 
  if (vp->v_op->vop_close == NULL)
@@ -184,6 +187,7 @@ VOP_ACCESS(struct vnode *vp, int mode, s
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  ASSERT_VP_ISLOCKED(vp);
 
  if (vp->v_op->vop_access == NULL)
@@ -202,6 +206,7 @@ VOP_GETATTR(struct vnode *vp, struct vat
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  if (vp->v_op->vop_getattr == NULL)
  return (EOPNOTSUPP);
 
@@ -219,6 +224,7 @@ VOP_SETATTR(struct vnode *vp, struct vat
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  ASSERT_VP_ISLOCKED(vp);
 
  if (vp->v_op->vop_setattr == NULL)
@@ -282,6 +288,7 @@ VOP_IOCTL(struct vnode *vp, u_long comma
  a.a_cred = cred;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  if (vp->v_op->vop_ioctl == NULL)
  return (EOPNOTSUPP);
 
@@ -300,6 +307,7 @@ VOP_POLL(struct vnode *vp, int fflag, in
  a.a_events = events;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  if (vp->v_op->vop_poll == NULL)
  return (EOPNOTSUPP);
 
@@ -343,6 +351,7 @@ VOP_FSYNC(struct vnode *vp, struct ucred
  a.a_waitfor = waitfor;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  ASSERT_VP_ISLOCKED(vp);
 
  if (vp->v_op->vop_fsync == NULL)
@@ -564,6 +573,7 @@ VOP_INACTIVE(struct vnode *vp, struct pr
  a.a_vp = vp;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  ASSERT_VP_ISLOCKED(vp);
 
  if (vp->v_op->vop_inactive == NULL)
@@ -580,6 +590,7 @@ VOP_RECLAIM(struct vnode *vp, struct pro
  a.a_vp = vp;
  a.a_p = p;
 
+ KASSERT(p == curproc);
  if (vp->v_op->vop_reclaim == NULL)
  return (EOPNOTSUPP);
 

Reply | Threaded
Open this post in threaded view
|

Re: Untangle proc * in VOP_*

Ted Unangst-6
On 2020-03-23, Martin Pieuchot wrote:

> Most of the VOP_* methods include an argument of type "struct proc *"
> called `a_p'.  This pointer is always set to `curproc' as confirmed by
> the diff below.  The diff has been though base build with NFS on amd64
> and sparc64 as well as a full port bulk on amd64 and is in the current
> octeon port bulk.
>
> I'd like to commit it in order to start removing the requirement of
> passing a "struct proc *" pointer which is always set to curproc.  This
> is unnecessary and has spread in many other places in the kernel.
>
> That makes codes unnecessarily complicated:
>  - a "struct proc *" is carried over just to satisfy already complex
>    interfaces
>  - it isn't obvious which "struct proc *" is not the curproc and
>    asserts like KASSERT(p == curproc) are being made
>  - in many places where curproc is used people put XXX as if it was
>    wrong
>
> So this is just a starting diff, cleanups can start afterward.

If this is a temporary measure, I think it should include some
annotation to that effect, so that it doesn't continue spreading.

Reply | Threaded
Open this post in threaded view
|

Re: Untangle proc * in VOP_*

Martin Pieuchot
In reply to this post by Martin Pieuchot
On 23/03/20(Mon) 18:51, Ted Unangst wrote:
>  [...]
> If this is a temporary measure, I think it should include some
> annotation to that effect, so that it doesn't continue spreading.

It isn't, it's to help review or upcoming diffs :o)  It's not to make
this nice it's to tedu it!