Bug#190732: [PATCH] hurd/libdiskfs/dir-renamed.c

[Ognyan Kulev]

Ognyan Kulev wrote:
> I'll try to address this and other problems 
> (like reverting back st_nlink when error occurs) in a "final" patch soon.

The patch is ready, but it doesn't revert back st_nlink:s.  There are 
many small changes so it would be good if someone else take a careful 
look too.  It features verbose changelog entries too.

Regards
--
Ognyan Kulev <ogi@{fmi.uni-sofia.bg,fsa-bg.org}>
7D9F 66E6 68B7 A62B 0FCF  EB04 80BF 3A8C A252 9782

2003-07-27  Ognyan Kulev  <ogi@fmi.uni-sofia.bg>

        * dir-renamed.c (checkpath): Redundant assignment is removed.
        (diskfs_rename_dir): Remove variable BUF.
        Space for DS is allocated with alloca.
        When renaming node to itself, goto out instead of repeating code.
        When checking if TNP is empty directory, assertion "!err" is
        checked.
        Check early if we can remove FROMNAME.  This implicitly locks FNP,
        so the explicit locking of FNP is removed.
        When maximum link count of TDP is reached, goto out instead of
        returning immediately.
        When maximum link count of FNP is reached, goto out instead of
        repeating code.
        diskfs_node_update FNP only if diskfs_synchronous.
        After entering entry TONAME to TDP, set DS to 0.
        When preparing to remove FROMNAME from FDP, use TMPDS instead of
        DS.  Fix the assertion so that it can handle errors.  If error
        occur, diskfs_drop_dirstat TMPDS before goto out.

        * dir-rename.c (diskfs_S_dir_rename): Now really serialize
        directory renaming with RENAMEDIRLOCK.
        When renaming directory, don't diskfs_{node,file}_update because
        dir-renamed.c::diskfs_rename_dir already do it.
        When TONAME is .. of filesystem's root, not only set ERR to
        EINVAL, but return this error.
        When EXCL is set and target exist, unlock things properly.
        When FROMNAME is regular file and TONAME is directory, unlock TNP
        before returning.
        When maximum link count of FNP is reached and TNP is not NULL,
        unlock TNP before returning.
        When updating FNP after incrementing its link count, call
        diskfs_node_update only if diskfs_synchronous.
        After TONAME enters TDP, call diskfs_file_update instead of
        diskfs_node_update.

diff -urpN --exclude=build --exclude='*~' --exclude=ChangeLog 
/home/ogi/cvs/hurd/libdiskfs/dir-rename.c hurd/libdiskfs/dir-rename.c
--- /home/ogi/cvs/hurd/libdiskfs/dir-rename.c   1997-02-14 03:19:23.000000000 
+0200
+++ hurd/libdiskfs/dir-rename.c 2003-07-27 22:05:11.000000000 +0300
@@ -1,5 +1,5 @@
 /* libdiskfs implementation of fs.defs: dir_rename
-   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997 Free Software Foundation
+   Copyright (C) 1992,1993,1994,1995,1996,1997,2003 Free Software Foundation
 
    This program is free software; you can redistribute it and/or
    modify it under the terms of the GNU General Public License as
@@ -52,7 +52,6 @@ diskfs_S_dir_rename (struct protid *from
   fdp = fromcred->po->np;
   tdp = tocred->po->np;
 
- try_again:
   /* Acquire the source; hold a reference to it.  This 
      will prevent anyone from deleting it before we create
      the new link. */
@@ -66,30 +65,26 @@ diskfs_S_dir_rename (struct protid *from
 
   if (S_ISDIR (fnp->dn_stat.st_mode))
     {
-      mutex_unlock (&fnp->lock);
-      if (!mutex_try_lock (&renamedirlock))
+      diskfs_nput (fnp);
+
+      mutex_lock (&renamedirlock);
+
+      /* We now lookup again, but with RENAMEDIRLOCK locked.  */
+      mutex_lock (&fdp->lock);
+      err = diskfs_lookup (fdp, fromname, LOOKUP, &fnp, 0, fromcred);
+      mutex_unlock (&fdp->lock);
+      if (err == EAGAIN)
+       err = EINVAL;
+      if (err)
        {
-         diskfs_nrele (fnp);
-         mutex_lock (&renamedirlock);
-         goto try_again;
+         mutex_unlock (&renamedirlock);
+         return err;
        }
+      mutex_unlock (&fnp->lock);
+
       err = diskfs_rename_dir (fdp, fnp, fromname, tdp, toname, fromcred,
                               tocred);
-      if (diskfs_synchronous)
-       {
-         mutex_lock (&fdp->lock);
-         diskfs_file_update (fdp, 1);
-         mutex_unlock (&fdp->lock);
-         
-         mutex_lock (&fnp->lock);
-         diskfs_file_update (fnp, 1);
-         mutex_unlock (&fnp->lock);
-
-         mutex_lock (&tdp->lock);
-         diskfs_file_update (tdp, 1);
-         mutex_unlock (&tdp->lock);
-       }
-      
+
       diskfs_nrele (fnp);
       mutex_unlock (&renamedirlock);
       if (!err)
@@ -107,11 +102,14 @@ diskfs_S_dir_rename (struct protid *from
   
   err = diskfs_lookup (tdp, toname, RENAME, &tnp, ds, tocred);
   if (err == EAGAIN)
-    err = EINVAL;
-  else if (!err && excl)
+    err = EINVAL;              /* .. of filesystem's root.  */
+  if (!err && excl)
     {
-      err = EEXIST;
+      diskfs_drop_dirstat (tdp, ds);
+      diskfs_nrele (fnp);
       diskfs_nput (tnp);
+      mutex_unlock (&tdp->lock);
+      return EEXIST;
     }
   if (err && err != ENOENT)
     {
@@ -138,6 +136,7 @@ diskfs_S_dir_rename (struct protid *from
     {
       diskfs_drop_dirstat (tdp, ds);
       diskfs_nrele (fnp);
+      diskfs_nput (tnp);
       mutex_unlock (&tdp->lock);
       return EISDIR;
     }
@@ -149,12 +148,15 @@ diskfs_S_dir_rename (struct protid *from
     {
       diskfs_drop_dirstat (tdp, ds);
       diskfs_nput (fnp);
+      if (tnp)
+       diskfs_nput (tnp);
       mutex_unlock (&tdp->lock);
       return EMLINK;
     }
   fnp->dn_stat.st_nlink++;
   fnp->dn_set_ctime = 1;
-  diskfs_node_update (fnp, 1);
+  if (diskfs_synchronous)
+    diskfs_node_update (fnp, 1);
 
   if (tnp)
     {
@@ -172,7 +174,7 @@ diskfs_S_dir_rename (struct protid *from
     err = diskfs_direnter (tdp, toname, fnp, ds, tocred);
 
   if (diskfs_synchronous)
-    diskfs_node_update (tdp, 1);
+    diskfs_file_update (tdp, 1);
 
   mutex_unlock (&tdp->lock);
   mutex_unlock (&fnp->lock);
@@ -185,7 +187,7 @@ diskfs_S_dir_rename (struct protid *from
   /* We now hold no locks */
 
   /* Now we remove the source.  Unfortunately, we haven't held 
-     fdp locked (nor could we), so someone else might have already
+     FDP locked (nor could we), so someone else might have already
      removed it. */
   mutex_lock (&fdp->lock);
   err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, ds, fromcred);
diff -urpN --exclude=build --exclude='*~' --exclude=ChangeLog 
/home/ogi/cvs/hurd/libdiskfs/dir-renamed.c hurd/libdiskfs/dir-renamed.c
--- /home/ogi/cvs/hurd/libdiskfs/dir-renamed.c  2001-10-12 05:49:17.000000000 
+0300
+++ hurd/libdiskfs/dir-renamed.c        2003-07-27 20:55:45.000000000 +0300
@@ -1,5 +1,5 @@
 /*
-   Copyright (C) 1994,95,96,97,98,99,2001 Free Software Foundation, Inc.
+   Copyright (C) 1994,95,96,97,98,99,2001,03 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or
    modify it under the terms of the GNU General Public License as
@@ -32,7 +32,6 @@ checkpath(struct node *source,
   error_t err;
   struct node *np;
 
-  np = target;
   for (np = target, err = 0;
        /* nothing */;
        /* This special lookup does a diskfs_nput on its first argument
@@ -73,9 +72,7 @@ diskfs_rename_dir (struct node *fdp, str
 {
   error_t err;
   struct node *tnp, *tmpnp;
-  void *buf = alloca (diskfs_dirstat_size);
-  struct dirstat *ds;
-  struct dirstat *tmpds;
+  struct dirstat *ds, *tmpds;
 
   mutex_lock (&tdp->lock);
   diskfs_nref (tdp);           /* reference and lock will get consumed by
@@ -85,48 +82,57 @@ diskfs_rename_dir (struct node *fdp, str
   if (err)
     return err;
 
-  /* Now, lock the parent directories.  This is legal because tdp is not
-     a child of fnp (guaranteed by checkpath above). */
+  /* Now, lock the parent directories.  This is legal because TDP is not
+     a child of FNP (guaranteed by checkpath above). */
   mutex_lock (&fdp->lock);
   if (fdp != tdp)
     mutex_lock (&tdp->lock);
 
   /* 1: Lookup target; if it exists, make sure it's an empty directory. */
-  ds = buf;
+  ds = alloca (diskfs_dirstat_size);
   err = diskfs_lookup (tdp, toname, RENAME, &tnp, ds, tocred);
   assert (err != EAGAIN);      /* <-> assert (TONAME != "..") */
-
   if (tnp == fnp)
+    /* Renaming node to itself.  */
     {
-      diskfs_drop_dirstat (tdp, ds);
-      diskfs_nput (tnp);
-      mutex_unlock (&tdp->lock);
-      if (fdp != tdp)
-       mutex_unlock (&fdp->lock);
-      return 0;
+      err = 0;
+      fnp = 0;                 /* Don't unlock the unlocked FNP.  */
+      goto out;
+    }
+  if (err && err != ENOENT)
+    {
+      fnp = 0;                 /* Don't unlock the unlocked FNP.  */
+      goto out;
     }
-
-  /* Now we can safely lock fnp */
-  mutex_lock (&fnp->lock);
 
   if (tnp)
     {
+      assert (!err);
       if (! S_ISDIR(tnp->dn_stat.st_mode))
        err = ENOTDIR;
       else if (!diskfs_dirempty (tnp, tocred))
        err = ENOTEMPTY;
+      if (err)
+       goto out;
     }
 
-  if (err && err != ENOENT)
+  /* 2: Check permissions of the node being moved, and lock FNP.  */
+  tmpds = alloca (diskfs_dirstat_size);
+  err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, tmpds, fromcred);
+  assert (!tmpnp || tmpnp == fnp);
+  diskfs_drop_dirstat (fdp, tmpds);
+  if (tmpnp)
+    diskfs_nput (tmpnp);
+  if (err)
     goto out;
 
-  /* 2: Set our .. to point to the new parent */
+  /* 3: Set our .. to point to the new parent */
   if (fdp != tdp)
     {
       if (tdp->dn_stat.st_nlink == diskfs_link_max - 1)
        {
          err = EMLINK;
-         return EMLINK;
+         goto out;
        }
       tdp->dn_stat.st_nlink++;
       tdp->dn_set_ctime = 1;
@@ -157,20 +163,17 @@ diskfs_rename_dir (struct node *fdp, str
     }
 
 
-  /* 3: Increment the link count on the node being moved and rewrite
-     tdp. */
+  /* 4: Increment the link count on the node being moved and rewrite
+     TDP. */
   if (fnp->dn_stat.st_nlink == diskfs_link_max - 1)
     {
-      mutex_unlock (&fnp->lock);
-      diskfs_drop_dirstat (tdp, ds);
-      mutex_unlock (&tdp->lock);
-      if (tnp)
-       diskfs_nput (tnp);
-      return EMLINK;
+      err = EMLINK;
+      goto out;
     }
   fnp->dn_stat.st_nlink++;
   fnp->dn_set_ctime = 1;
-  diskfs_node_update (fnp, 1);
+  if (diskfs_synchronous)
+    diskfs_node_update (fnp, 1);
 
   if (tnp)
     {
@@ -188,6 +191,7 @@ diskfs_rename_dir (struct node *fdp, str
   else
     {
       err = diskfs_direnter (tdp, toname, fnp, ds, tocred);
+      ds = 0;
       if (diskfs_synchronous)
        diskfs_file_update (tdp, 1);
     }
@@ -195,17 +199,19 @@ diskfs_rename_dir (struct node *fdp, str
   if (err)
     goto out;
 
-  /* 4: Remove the entry in fdp. */
-  ds = buf;
+  /* 5: Remove the entry from FDP. */
   mutex_unlock (&fnp->lock);
-  err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, ds, fromcred);
-  assert (tmpnp == fnp);
-  diskfs_nrele (tmpnp);
+  err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, tmpds, fromcred);
+  assert (!tmpnp || tmpnp == fnp);
+  if (tmpnp)
+    diskfs_nput (tmpnp);
   if (err)
-    goto out;
+    {
+      diskfs_drop_dirstat (fdp, tmpds);
+      goto out;
+    }
 
-  diskfs_dirremove (fdp, fnp, fromname, ds);
-  ds = 0;
+  diskfs_dirremove (fdp, fnp, fromname, tmpds);
   fnp->dn_stat.st_nlink--;
   fnp->dn_set_ctime = 1;
   if (diskfs_synchronous)