Subject: Re: vop_symlink and unused vpp?
To: Bill Studenmund <wrstuden@zembu.com>
From: Assar Westerlund <assar@netbsd.org>
List: tech-kern
Date: 07/21/2001 04:17:45
--=-=-=
I wrote:
> Yes, it seems simpler always doing that. And it will probably make
> layer (or whoever is going to debug that) happier too. I'll send a
> patch that implements my original proposed choice c) tomorrow, after I
> have gotten some sleep.
Here's a patch for that. I'll commit it unless anyone has objections.
/assar
--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=netbsd-sys3.diff
Index: coda/coda_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/coda/coda_vnops.c,v
retrieving revision 1.25
diff -u -w -r1.25 coda_vnops.c
--- coda/coda_vnops.c 2001/07/03 06:46:52 1.25
+++ coda/coda_vnops.c 2001/07/21 01:40:02
@@ -1655,21 +1655,6 @@
}
/*
- * Okay, now we have to drop locks on dvp. vpp is unlocked, but
- * ref'd. It doesn't matter what happens in either symlink or
- * lookup. Furthermore, there isn't any way for (dvp == *vpp), so
- * we don't bother checking.
- */
-/* vput(ap->a_dvp); released earlier */
- if (*ap->a_vpp) {
- VOP_UNLOCK(*ap->a_vpp, 0); /* this line is new!! It is necessary because lookup() calls
- VOP_LOOKUP (coda_lookup) which returns vpp locked. cfs_nb_lookup
- merged with coda_lookup() to become coda_lookup so UNLOCK is
- necessary */
- vrele(*ap->a_vpp);
- }
-
- /*
* Free the name buffer
*/
if ((cnp->cn_flags & SAVESTART) == 0) {
Index: kern/vfs_syscalls.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/vfs_syscalls.c,v
retrieving revision 1.167
diff -u -w -r1.167 vfs_syscalls.c
--- kern/vfs_syscalls.c 2001/06/28 08:04:18 1.167
+++ kern/vfs_syscalls.c 2001/07/21 01:40:32
@@ -1381,6 +1381,8 @@
} else {
error = VOP_MKNOD(nd.ni_dvp, &nd.ni_vp,
&nd.ni_cnd, &vattr);
+ if (error == 0)
+ vput(nd.ni_vp);
}
} else {
VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
@@ -1428,7 +1430,10 @@
vattr.va_type = VFIFO;
vattr.va_mode = (SCARG(uap, mode) & ALLPERMS) &~ p->p_cwdi->cwdi_cmask;
VOP_LEASE(nd.ni_dvp, p, p->p_ucred, LEASE_WRITE);
- return (VOP_MKNOD(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr));
+ error = VOP_MKNOD(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr);
+ if (error == 0)
+ vput(nd.ni_vp);
+ return (error);
}
/*
@@ -1514,6 +1519,8 @@
vattr.va_mode = ACCESSPERMS &~ p->p_cwdi->cwdi_cmask;
VOP_LEASE(nd.ni_dvp, p, p->p_ucred, LEASE_WRITE);
error = VOP_SYMLINK(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr, path);
+ if (error == 0)
+ vput(nd.ni_vp);
out:
PNBUF_PUT(path);
return (error);
Index: kern/vnode_if.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/vnode_if.c,v
retrieving revision 1.36
diff -u -w -r1.36 vnode_if.c
--- kern/vnode_if.c 2001/05/26 21:34:04 1.36
+++ kern/vnode_if.c 2001/07/21 01:40:32
@@ -198,7 +198,7 @@
const struct vnodeop_desc vop_mknod_desc = {
5,
"vop_mknod",
- 0 | VDESC_VP0_WILLPUT | VDESC_VPP_WILLRELE,
+ 0 | VDESC_VP0_WILLPUT,
vop_mknod_vp_offsets,
VOPARG_OFFSETOF(struct vop_mknod_args, a_vpp),
VDESC_NO_OFFSET,
@@ -868,7 +868,7 @@
const struct vnodeop_desc vop_symlink_desc = {
25,
"vop_symlink",
- 0 | VDESC_VP0_WILLPUT | VDESC_VPP_WILLRELE,
+ 0 | VDESC_VP0_WILLPUT,
vop_symlink_vp_offsets,
VOPARG_OFFSETOF(struct vop_symlink_args, a_vpp),
VDESC_NO_OFFSET,
Index: kern/vnode_if.src
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/vnode_if.src,v
retrieving revision 1.28
diff -u -w -r1.28 vnode_if.src
--- kern/vnode_if.src 2001/05/26 21:33:11 1.28
+++ kern/vnode_if.src 2001/07/21 01:40:33
@@ -113,13 +113,13 @@
#
#% mknod dvp L U U
-#% mknod vpp - X -
+#% mknod vpp - L -
#
#! mknod cnp CREATE, LOCKPARENT
#
vop_mknod {
IN WILLPUT struct vnode *dvp;
- OUT WILLRELE struct vnode **vpp;
+ OUT struct vnode **vpp;
IN struct componentname *cnp;
IN struct vattr *vap;
};
@@ -338,17 +338,13 @@
#
#% symlink dvp L U U
-#% symlink vpp - U -
+#% symlink vpp - L -
#
#! symlink cnp CREATE, LOCKPARENT
#
-# XXX - note that the return vnode has already been VRELE'ed
-# by the filesystem layer. To use it you must use vget,
-# possibly with a further namei.
-#
vop_symlink {
IN WILLPUT struct vnode *dvp;
- OUT WILLRELE struct vnode **vpp;
+ OUT struct vnode **vpp;
IN struct componentname *cnp;
IN struct vattr *vap;
IN char *target;
Index: lkm/netinet/if_ipl/mln_ipl.c
===================================================================
RCS file: /cvsroot/syssrc/sys/lkm/netinet/if_ipl/mln_ipl.c,v
retrieving revision 1.24
diff -u -w -r1.24 mln_ipl.c
--- lkm/netinet/if_ipl/mln_ipl.c 2001/02/05 10:42:42 1.24
+++ lkm/netinet/if_ipl/mln_ipl.c 2001/07/21 01:40:47
@@ -253,6 +253,7 @@
error = VOP_MKNOD(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr);
if (error)
return error;
+ vput(nd.ni_vp);
}
return error;
}
Index: miscfs/genfs/layer_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/miscfs/genfs/layer_vnops.c,v
retrieving revision 1.6
diff -u -w -r1.6 layer_vnops.c
--- miscfs/genfs/layer_vnops.c 2001/06/07 13:32:47 1.6
+++ miscfs/genfs/layer_vnops.c 2001/07/21 01:40:48
@@ -393,8 +393,7 @@
descp->vdesc_vpp_offset,ap);
/*
* Only vop_lookup, vop_create, vop_makedir, vop_bmap,
- * vop_mknod, and vop_symlink return vpp's. The latter
- * two are VPP_WILLRELE, so we won't get here, and vop_bmap
+ * vop_mknod, and vop_symlink return vpp's. vop_bmap
* doesn't call bypass as the lower vpp is fine (we're just
* going to do i/o on it). vop_loookup doesn't call bypass
* as a lookup on "." would generate a locking error.
Index: miscfs/union/union_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/miscfs/union/union_vnops.c,v
retrieving revision 1.53
diff -u -w -r1.53 union_vnops.c
--- miscfs/union/union_vnops.c 2001/07/04 21:38:00 1.53
+++ miscfs/union/union_vnops.c 2001/07/21 01:40:52
@@ -610,12 +610,10 @@
if (error)
return (error);
- if (vp != NULLVP) {
error = union_allocvp(ap->a_vpp, mp, NULLVP, NULLVP,
cnp, vp, NULLVP, 1);
if (error)
vput(vp);
- }
return (error);
}
@@ -1527,14 +1525,13 @@
if (dvp != NULLVP) {
int error;
- struct vnode *vp;
FIXUP(un);
VREF(dvp);
un->un_flags |= UN_KLOCK;
vput(ap->a_dvp);
- error = VOP_SYMLINK(dvp, &vp, cnp, ap->a_vap, ap->a_target);
- *ap->a_vpp = NULLVP;
+ error = VOP_SYMLINK(dvp, ap->a_vpp, cnp, ap->a_vap,
+ ap->a_target);
return (error);
}
Index: nfs/nfs_serv.c
===================================================================
RCS file: /cvsroot/syssrc/sys/nfs/nfs_serv.c,v
retrieving revision 1.59
diff -u -w -r1.59 nfs_serv.c
--- nfs/nfs_serv.c 2000/11/27 08:39:49 1.59
+++ nfs/nfs_serv.c 2001/07/21 01:40:55
@@ -1417,15 +1417,7 @@
vrele(nd.ni_startdir);
nfsm_reply(0);
}
- nd.ni_cnd.cn_nameiop = LOOKUP;
- nd.ni_cnd.cn_flags &= ~(LOCKPARENT | SAVESTART);
- nd.ni_cnd.cn_proc = procp;
- nd.ni_cnd.cn_cred = cred;
- if ((error = lookup(&nd)) != 0) {
PNBUF_PUT(nd.ni_cnd.cn_pnbuf);
- nfsm_reply(0);
- }
- PNBUF_PUT(nd.ni_cnd.cn_pnbuf);
if (nd.ni_cnd.cn_flags & ISSYMLINK) {
vrele(nd.ni_dvp);
vput(nd.ni_vp);
@@ -1618,11 +1610,6 @@
vrele(nd.ni_startdir);
goto out;
}
- nd.ni_cnd.cn_nameiop = LOOKUP;
- nd.ni_cnd.cn_flags &= ~(LOCKPARENT | SAVESTART);
- nd.ni_cnd.cn_proc = procp;
- nd.ni_cnd.cn_cred = procp->p_ucred;
- error = lookup(&nd);
PNBUF_PUT(nd.ni_cnd.cn_pnbuf);
if (error)
goto out;
@@ -2146,13 +2133,6 @@
vrele(nd.ni_startdir);
else {
if (v3) {
- nd.ni_cnd.cn_nameiop = LOOKUP;
- nd.ni_cnd.cn_flags &= ~(LOCKPARENT | SAVESTART | FOLLOW);
- nd.ni_cnd.cn_flags |= (NOFOLLOW | LOCKLEAF);
- nd.ni_cnd.cn_proc = procp;
- nd.ni_cnd.cn_cred = cred;
- error = lookup(&nd);
- if (!error) {
memset((caddr_t)fhp, 0, sizeof(nfh));
fhp->fh_fsid = nd.ni_vp->v_mount->mnt_stat.f_fsid;
error = VFS_VPTOFH(nd.ni_vp, &fhp->fh_fid);
@@ -2160,9 +2140,10 @@
error = VOP_GETATTR(nd.ni_vp, &va, cred,
procp);
vput(nd.ni_vp);
- }
- } else
+ } else {
vrele(nd.ni_startdir);
+ vput(nd.ni_vp);
+ }
PNBUF_PUT(nd.ni_cnd.cn_pnbuf);
}
out:
Index: nfs/nfs_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/nfs/nfs_vnops.c,v
retrieving revision 1.134
diff -u -w -r1.134 nfs_vnops.c
--- nfs/nfs_vnops.c 2001/06/07 01:04:40 1.134
+++ nfs/nfs_vnops.c 2001/07/21 01:40:57
@@ -1407,8 +1407,6 @@
int error;
error = nfs_mknodrpc(ap->a_dvp, &newvp, ap->a_cnp, ap->a_vap);
- if (!error)
- vput(newvp);
return (error);
}
@@ -1869,6 +1867,7 @@
struct vnode *newvp = (struct vnode *)0;
const int v3 = NFS_ISV3(dvp);
+ *ap->a_vpp = NULL;
nfsstats.rpccnt[NFSPROC_SYMLINK]++;
slen = strlen(ap->a_target);
nfsm_reqhead(dvp, NFSPROC_SYMLINK, NFSX_FH(v3) + 2*NFSX_UNSIGNED +
@@ -1894,18 +1893,30 @@
nfsm_wcc_data(dvp, wccflag);
}
nfsm_reqdone;
- if (newvp)
+ /*
+ * Kludge: Map EEXIST => 0 assuming that it is a reply to a retry.
+ */
+ if (error == EEXIST)
+ error = 0;
+ if (error == 0 && newvp == NULL) {
+ struct nfsnode *np = NULL;
+
+ np = nfs_lookitup(dvp, cnp->cn_nameptr, cnp->cn_namelen,
+ cnp->cn_cred, cnp->cn_proc, &np);
+ if (error == 0)
+ newvp = NFSTOV(np);
+ }
+ if (error) {
+ if (newvp != NULL)
vput(newvp);
+ } else {
+ *ap->a_vpp = newvp;
+ }
PNBUF_PUT(cnp->cn_pnbuf);
VTONFS(dvp)->n_flag |= NMODIFIED;
if (!wccflag)
VTONFS(dvp)->n_attrstamp = 0;
vput(dvp);
- /*
- * Kludge: Map EEXIST => 0 assuming that it is a reply to a retry.
- */
- if (error == EEXIST)
- error = 0;
return (error);
}
Index: ufs/ext2fs/ext2fs_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/ext2fs/ext2fs_vnops.c,v
retrieving revision 1.33
diff -u -w -r1.33 ext2fs_vnops.c
--- ufs/ext2fs/ext2fs_vnops.c 2001/03/23 21:11:08 1.33
+++ ufs/ext2fs/ext2fs_vnops.c 2001/07/21 01:40:59
@@ -132,11 +132,15 @@
struct vnode **vpp = ap->a_vpp;
struct inode *ip;
int error;
+ struct mount *mp;
+ ino_t ino;
if ((error = ext2fs_makeinode(MAKEIMODE(vap->va_type, vap->va_mode),
ap->a_dvp, vpp, ap->a_cnp)) != 0)
return (error);
ip = VTOI(*vpp);
+ mp = (*vpp)->v_mount;
+ ino = ip->i_number;
ip->i_flag |= IN_ACCESS | IN_CHANGE | IN_UPDATE;
if (vap->va_rdev != VNOVAL) {
/*
@@ -153,7 +157,11 @@
vput(*vpp);
(*vpp)->v_type = VNON;
vgone(*vpp);
- *vpp = 0;
+ error = VFS_VGET(mp, ino, vpp);
+ if (error != 0) {
+ *vpp = NULL;
+ return (error);
+ }
return (0);
}
@@ -1206,6 +1214,7 @@
error = vn_rdwr(UIO_WRITE, vp, ap->a_target, len, (off_t)0,
UIO_SYSSPACE, IO_NODELOCKED, ap->a_cnp->cn_cred,
(size_t *)0, (struct proc *)0);
+ if (error)
vput(vp);
return (error);
}
Index: ufs/lfs/lfs_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/lfs/lfs_vnops.c,v
retrieving revision 1.51
diff -u -w -r1.51 lfs_vnops.c
--- ufs/lfs/lfs_vnops.c 2001/07/13 20:30:25 1.51
+++ ufs/lfs/lfs_vnops.c 2001/07/21 01:40:59
@@ -450,6 +450,8 @@
struct vnode **vpp = ap->a_vpp;
struct inode *ip;
int error;
+ struct mount *mp;
+ ino_t ino;
if ((error = SET_DIROP(ap->a_dvp)) != 0) {
vput(ap->a_dvp);
@@ -469,6 +471,8 @@
return (error);
ip = VTOI(*vpp);
+ mp = (*vpp)->v_mount;
+ ino = ip->i_number;
ip->i_flag |= IN_ACCESS | IN_CHANGE | IN_UPDATE;
if (vap->va_rdev != VNOVAL) {
/*
@@ -505,7 +509,11 @@
lfs_vunref(*vpp);
(*vpp)->v_type = VNON;
vgone(*vpp);
- *vpp = 0;
+ error = VFS_VGET(mp, ino, vpp);
+ if (error != 0) {
+ *vpp = NULL;
+ return (error);
+ }
return (0);
}
Index: ufs/ufs/ufs_vnops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.78
diff -u -w -r1.78 ufs_vnops.c
--- ufs/ufs/ufs_vnops.c 2001/05/28 02:50:53 1.78
+++ ufs/ufs/ufs_vnops.c 2001/07/21 01:41:00
@@ -138,6 +138,8 @@
struct vnode **vpp;
struct inode *ip;
int error;
+ struct mount *mp;
+ ino_t ino;
vap = ap->a_vap;
vpp = ap->a_vpp;
@@ -146,6 +148,8 @@
ap->a_dvp, vpp, ap->a_cnp)) != 0)
return (error);
ip = VTOI(*vpp);
+ mp = (*vpp)->v_mount;
+ ino = ip->i_number;
ip->i_flag |= IN_ACCESS | IN_CHANGE | IN_UPDATE;
if (vap->va_rdev != VNOVAL) {
/*
@@ -153,7 +157,7 @@
* inodes, so don't truncate the dev number.
*/
ip->i_ffs_rdev = ufs_rw32(vap->va_rdev,
- UFS_MPNEEDSWAP((*vpp)->v_mount));
+ UFS_MPNEEDSWAP(mp);
}
/*
* Remove inode so that it will be reloaded by VFS_VGET and
@@ -163,7 +167,11 @@
vput(*vpp);
(*vpp)->v_type = VNON;
vgone(*vpp);
- *vpp = 0;
+ error = VFS_VGET(mp, ino, vpp);
+ if (error != 0) {
+ *vpp = NULL;
+ return (error);
+ }
return (0);
}
@@ -1450,6 +1458,7 @@
error = vn_rdwr(UIO_WRITE, vp, ap->a_target, len, (off_t)0,
UIO_SYSSPACE, IO_NODELOCKED, ap->a_cnp->cn_cred, NULL,
(struct proc *)0);
+ if (error)
vput(vp);
return (error);
}
--=-=-=--