rename bug in arla

[Love]

Chuck Lever <chuckl at netscape.com> writes:

I can't reproduce the error.

> however, i found a logic bug in arlad's rename operation.  in
> arlad/inter.c, the logic first creates the entry for the new name in the
> cached copy of the new directory, then deletes the old one.  the problem
> occurs if an entry with the new name already exists.

In that case the rename_file() inter.c:988 would have failed. But that
possible to get a callback some where inbetween.

Try the patch below, it might do a diffrence.

If you have a test-case I'ld like to see it. 

Love


Index: adir.c
===================================================================
RCS file: /usr/local/cvsroot/arla/arlad/adir.c,v
retrieving revision 1.48
diff -u -w -r1.48 adir.c
--- adir.c	1998/11/10 04:07:39	1.48
+++ adir.c	1998/11/14 03:58:38
@@ -287,38 +287,31 @@
 }
 
 /*
- * Lookup `name' in the AFS directory identified by `dir' and return
+ * Lookup `name' in the AFS directory identified by `centry' and return
  * the Fid in `file'.  All operations are done as `cred' and return
  * value is 0 or error code.
+ *
+ *
+ * Locking:
+ *            In        Out       Fail
+ *    centry: Locked    Locked    Locked
+ *  
  */
 
 int
-adir_lookup (VenusFid dir,
+adir_lookup_fcacheentry (FCacheEntry *centry,
 	     const char *name,
 	     VenusFid *file,
 	     CredCacheEntry *ce)
 {
      int fd;
      DirPage0 *page0;
-     FCacheEntry *centry = NULL;
      unsigned ind;
      unsigned len;
      int ret;
      fbuf the_fbuf;
      struct stat sb;
 
-     ret = fcache_get (&centry, dir, ce);
-     if (ret)
-	 return ret;
-
-     assert (CheckLock (&centry->lock) == -1);
-
-     ret = fcache_get_data (centry, ce);
-     if (ret) {
-	 fcache_release (centry);
-	 return ret;
-     }
-
      assert (CheckLock (&centry->lock) == -1);
 
      if (centry->status.FileType != TYPE_DIR) {
@@ -358,16 +351,53 @@
 
      assert (CheckLock (&centry->lock) == -1);
 
-     fcache_release(centry);
-
      page0 = (DirPage0 *)(the_fbuf.buf);
-     ind = find_by_name (page0, name, file, &dir);
+     ind = find_by_name (page0, name, file, &centry->fid);
      fbuf_end (&the_fbuf);
 
      if (ind == 0)
 	 return 0;
      else
 	 return ENOENT;
+}
+
+/*
+ * Lookup `name' in the AFS directory identified by `dir' and return
+ * the Fid in `file'.  All operations are done as `cred' and return
+ * value is 0 or error code.
+ *
+ * Locking:
+ *            In        Out       Fail
+ *       dir: Unlocked  Unlocked  Unlocked
+ */
+
+int
+adir_lookup (VenusFid dir,
+	     const char *name,
+	     VenusFid *file,
+	     CredCacheEntry *ce)
+{
+    FCacheEntry *centry;
+    int ret;
+
+    ret = fcache_get (&centry, dir, ce);
+    if (ret)
+	return ret;
+    
+    assert (CheckLock (&centry->lock) == -1);
+    
+    ret = fcache_get_data (centry, ce);
+    if (ret) {
+	fcache_release (centry);
+	return ret;
+    }
+    
+    ret = adir_lookup_fcacheentry (centry, name, file, ce);
+
+    fcache_release (centry);
+    
+    return ret;
+	
 }
 
 /*
Index: adir.h
===================================================================
RCS file: /usr/local/cvsroot/arla/arlad/adir.h,v
retrieving revision 1.9
diff -u -w -r1.9 adir.h
--- adir.h	1998/10/18 23:47:02	1.9
+++ adir.h	1998/11/14 03:58:38
@@ -53,6 +53,10 @@
 		 CredCacheEntry *ce);
 
 int
+adir_lookup_fcacheentry (FCacheEntry *centry,  const char *name,
+			 VenusFid *file, CredCacheEntry *ce);
+
+int
 adir_changefid (VenusFid dir,
 		const char *name,
 		VenusFid *file,
Index: inter.c
===================================================================
RCS file: /usr/local/cvsroot/arla/arlad/inter.c,v
retrieving revision 1.59
diff -u -w -r1.59 inter.c
--- inter.c	1998/10/31 02:42:21	1.59
+++ inter.c	1998/11/14 03:58:38
@@ -994,9 +994,7 @@
 	  } else {
 	      VenusFid foo_fid;
 
-	      ReleaseWriteLock (&old_dir->lock); /* XXX */
-	      error = adir_lookup (old_dir->fid, old_name, &foo_fid, ce);
-	      ObtainWriteLock (&old_dir->lock);	/* XXX */
+	      error = adir_lookup_fcacheentry (old_dir, old_name, &foo_fid, ce);
 
 	      if (error) {
 		  ret.res = -1;