Subject: Re: kern/33861
To: None <gnats-bugs@NetBSD.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: netbsd-bugs
Date: 06/30/2006 18:39:20
--NextPart-20060630183347-0368000
Content-Type: Text/Plain; charset=us-ascii

>  It's still worthwhile to double-check the existence of the source file
>  before going ahead with the sillyrename, I feel, to avoid unnecessary
>  silliness.  (It also happens that OpenSolaris does this, too.)

i'm not sure if it's a good idea.
isn't it a matter of VOP interface difference rather than carefulness?

>  So, I've included a patch which does those two things; I've done some
>  simple tests with it, but it could use more looking-at, as I had to
>  outline from nfs_link() the code that does the actual RPC, as opposed to
>  vnode glue.

coincidently i've written the similar patch.  (attached)
next time, let's take gnats "Responsible:" before fixing. :-)

YAMAMOTO Takashi

--NextPart-20060630183347-0368000
Content-Type: Text/Plain; charset=us-ascii
Content-Disposition: attachment; filename="a.diff"

Index: nfs_var.h
===================================================================
--- nfs_var.h	(revision 1657)
+++ nfs_var.h	(working copy)
@@ -119,7 +119,8 @@ int nfs_renamerpc(struct vnode *, const 
 	const char *, int, kauth_cred_t, struct lwp *);
 int nfs_readdirrpc(struct vnode *, struct uio *, kauth_cred_t);
 int nfs_readdirplusrpc(struct vnode *, struct uio *, kauth_cred_t);
-int nfs_sillyrename(struct vnode *, struct vnode *, struct componentname *);
+int nfs_sillyrename(struct vnode *, struct vnode *, struct componentname *,
+	boolean_t);
 int nfs_lookitup(struct vnode *, const char *, int, kauth_cred_t,
 	struct lwp *, struct nfsnode **);
 int nfs_commit(struct vnode *, off_t, uint32_t, struct lwp *);
Index: nfs_vnops.c
===================================================================
--- nfs_vnops.c	(revision 1702)
+++ nfs_vnops.c	(working copy)
@@ -239,6 +239,8 @@ const struct vnodeopv_entry_desc fifo_nf
 const struct vnodeopv_desc fifo_nfsv2nodeop_opv_desc =
 	{ &fifo_nfsv2nodeop_p, fifo_nfsv2nodeop_entries };
 
+static int nfs_linkrpc(struct vnode *, struct vnode *, const char *,
+    size_t, kauth_cred_t, struct lwp *);
 static void nfs_writerpc_extfree(struct mbuf *, caddr_t, size_t, void *);
 
 /*
@@ -1883,7 +1885,7 @@ nfs_remove(v)
 			printf("nfs_remove: vnode still has %u pages\n",
 			    vp->v_uobj.uo_npages);
 	} else if (!np->n_sillyrename)
-		error = nfs_sillyrename(dvp, vp, cnp);
+		error = nfs_sillyrename(dvp, vp, cnp, FALSE);
 	PNBUF_PUT(cnp->cn_pnbuf);
 	if (!error && nfs_getattrcache(vp, &vattr) == 0 &&
 	    vattr.va_nlink == 1) {
@@ -2002,7 +2004,7 @@ nfs_rename(v)
 	 * rename of the new file over it.
 	 */
 	if (tvp && tvp->v_usecount > 1 && !VTONFS(tvp)->n_sillyrename &&
-		tvp->v_type != VDIR && !nfs_sillyrename(tdvp, tvp, tcnp)) {
+	    tvp->v_type != VDIR && !nfs_sillyrename(tdvp, tvp, tcnp, TRUE)) {
 		VN_KNOTE(tvp, NOTE_DELETE);
 		vput(tvp);
 		tvp = NULL;
@@ -2107,6 +2109,54 @@ nfs_renamerpc(fdvp, fnameptr, fnamelen, 
 	return (error);
 }
 
+static int
+nfs_linkrpc(struct vnode *dvp, struct vnode *vp, const char *name,
+    size_t namelen, kauth_cred_t cred, struct lwp *l)
+{
+	u_int32_t *tl;
+	caddr_t cp;
+#ifndef NFS_V2_ONLY
+	int32_t t1;
+	caddr_t cp2;
+#endif
+	int32_t t2;
+	caddr_t bpos, dpos;
+	int error = 0, wccflag = NFSV3_WCCRATTR, attrflag = 0;
+	struct mbuf *mreq, *mrep, *md, *mb;
+	const int v3 = NFS_ISV3(dvp);
+	int rexmit = 0;
+	struct nfsnode *np = VTONFS(vp);
+
+	nfsstats.rpccnt[NFSPROC_LINK]++;
+	nfsm_reqhead(np, NFSPROC_LINK,
+	    NFSX_FH(v3)*2 + NFSX_UNSIGNED + nfsm_rndup(namelen));
+	nfsm_fhtom(np, v3);
+	nfsm_fhtom(VTONFS(dvp), v3);
+	nfsm_strtom(name, namelen, NFS_MAXNAMLEN);
+	nfsm_request1(np, NFSPROC_LINK, l, cred, &rexmit);
+#ifndef NFS_V2_ONLY
+	if (v3) {
+		nfsm_postop_attr(vp, attrflag, 0);
+		nfsm_wcc_data(dvp, wccflag, 0, !error);
+	}
+#endif
+	nfsm_reqdone;
+
+	VTONFS(dvp)->n_flag |= NMODIFIED;
+	if (!attrflag)
+		NFS_INVALIDATE_ATTRCACHE(VTONFS(vp));
+	if (!wccflag)
+		NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
+
+	/*
+	 * Kludge: Map EEXIST => 0 assuming that it is a reply to a retry.
+	 */
+	if (rexmit && error == EEXIST)
+		error = 0;
+
+	return error;
+}
+
 /*
  * nfs hard link create call
  */
@@ -2122,20 +2172,7 @@ nfs_link(v)
 	struct vnode *vp = ap->a_vp;
 	struct vnode *dvp = ap->a_dvp;
 	struct componentname *cnp = ap->a_cnp;
-	u_int32_t *tl;
-	caddr_t cp;
-#ifndef NFS_V2_ONLY
-	int32_t t1;
-	caddr_t cp2;
-#endif
-	int32_t t2;
-	caddr_t bpos, dpos;
-	int error = 0, wccflag = NFSV3_WCCRATTR, attrflag = 0;
-	struct mbuf *mreq, *mrep, *md, *mb;
-	/* XXX Should be const and initialised? */
-	int v3;
-	int rexmit = 0;
-	struct nfsnode *np;
+	int error = 0;
 
 	if (dvp->v_mount != vp->v_mount) {
 		VOP_ABORTOP(dvp, cnp);
@@ -2158,40 +2195,17 @@ nfs_link(v)
 	 */
 	VOP_FSYNC(vp, cnp->cn_cred, FSYNC_WAIT, 0, 0, cnp->cn_lwp);
 
-	v3 = NFS_ISV3(vp);
-	nfsstats.rpccnt[NFSPROC_LINK]++;
-	np = VTONFS(vp);
-	nfsm_reqhead(np, NFSPROC_LINK,
-		NFSX_FH(v3)*2 + NFSX_UNSIGNED + nfsm_rndup(cnp->cn_namelen));
-	nfsm_fhtom(np, v3);
-	nfsm_fhtom(VTONFS(dvp), v3);
-	nfsm_strtom(cnp->cn_nameptr, cnp->cn_namelen, NFS_MAXNAMLEN);
-	nfsm_request1(np, NFSPROC_LINK, cnp->cn_lwp, cnp->cn_cred, &rexmit);
-#ifndef NFS_V2_ONLY
-	if (v3) {
-		nfsm_postop_attr(vp, attrflag, 0);
-		nfsm_wcc_data(dvp, wccflag, 0, !error);
-	}
-#endif
-	nfsm_reqdone;
-	if (error == 0 || error == EEXIST)
+	error = nfs_linkrpc(dvp, vp, cnp->cn_nameptr, cnp->cn_namelen,
+	    cnp->cn_cred, cnp->cn_lwp);
+
+	if (error == 0)
 		cache_purge1(dvp, cnp, 0);
 	PNBUF_PUT(cnp->cn_pnbuf);
-	VTONFS(dvp)->n_flag |= NMODIFIED;
-	if (!attrflag)
-		NFS_INVALIDATE_ATTRCACHE(VTONFS(vp));
-	if (!wccflag)
-		NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
 	if (dvp != vp)
 		VOP_UNLOCK(vp, 0);
 	VN_KNOTE(vp, NOTE_LINK);
 	VN_KNOTE(dvp, NOTE_WRITE);
 	vput(dvp);
-	/*
-	 * Kludge: Map EEXIST => 0 assuming that it is a reply to a retry.
-	 */
-	if (rexmit && error == EEXIST)
-		error = 0;
 	return (error);
 }
 
@@ -3000,9 +3014,10 @@ nfsmout:
  * nfs_rename() completes, but...
  */
 int
-nfs_sillyrename(dvp, vp, cnp)
+nfs_sillyrename(dvp, vp, cnp, dolink)
 	struct vnode *dvp, *vp;
 	struct componentname *cnp;
+	boolean_t dolink;
 {
 	struct sillyrename *sp;
 	struct nfsnode *np;
@@ -3039,7 +3054,18 @@ nfs_sillyrename(dvp, vp, cnp)
 			goto bad;
 		}
 	}
-	error = nfs_renameit(dvp, cnp, sp);
+	if (dolink) {
+		error = nfs_linkrpc(dvp, vp, sp->s_name, sp->s_namlen,
+		    sp->s_cred, cnp->cn_lwp);
+		/*
+		 * nfs_request maps NFSERR_NOTSUPP to ENOTSUP.
+		 */
+		if (error == ENOTSUP) {
+			error = nfs_renameit(dvp, cnp, sp);
+		}
+	} else {
+		error = nfs_renameit(dvp, cnp, sp);
+	}
 	if (error)
 		goto bad;
 	error = nfs_lookitup(dvp, sp->s_name, sp->s_namlen, sp->s_cred,

--NextPart-20060630183347-0368000--