commit f424e4672ac1d9e1d78d9c760e55949d5da12e38

Author: Vinod Kumar <vinodkv@yahoo-inc.com>
Date:   Fri May 7 12:37:30 2010 +0530

    HADOOP-6631. From https://issues.apache.org/jira/secure/attachment/12443931/HADOOP-6631-20100506-ydist.final.txt
    
    +++ b/YAHOO-CHANGES.txt
    +    HADOOP-6631. FileUtil.fullyDelete() should continue to delete other files
    +    despite failure at any level. (vinodkv)
    +
    +    MAPREDUCE-1754. Replace mapred.persmissions.supergroup with an acl :
    +    mapreduce.cluster.administrators and HADOOP-6748.: Remove
    +    hadoop.cluster.administrators. Contributed by Amareshwari Sriramadasu.
    +
    +    MAPREDUCE-1707. TaskRunner can get NPE in getting ugi from TaskTracker. (vinodkv)
    +


git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.20-security-patches@1077448 13f79535-47bb-0310-9956-ffa450edef68

src/core/org/apache/hadoop/fs/FileUtil.java

@@ -82,12 +82,14 @@ public class FileUtil {
    * we return false, the directory may be partially-deleted.
    */
   public static boolean fullyDeleteContents(File dir) throws IOException {
+    boolean deletionSucceeded = true;
     File contents[] = dir.listFiles();
     if (contents != null) {
       for (int i = 0; i < contents.length; i++) {
         if (contents[i].isFile()) {
           if (!contents[i].delete()) {
-            return false;
+            deletionSucceeded = false;
+            continue; // continue deletion of other files/dirs under dir
           }
         } else {
           //try deleting the directory
@@ -101,12 +103,13 @@ public class FileUtil {
           // if not an empty directory or symlink let
           // fullydelete handle it.
           if (!fullyDelete(contents[i])) {
-            return false;
+            deletionSucceeded = false;
+            continue; // continue deletion of other files/dirs under dir
           }
         }
       }
     }
-    return true;
+    return deletionSucceeded;
   }
 
   /**

src/test/org/apache/hadoop/fs/TestFileUtil.java

@@ -19,20 +19,26 @@ package org.apache.hadoop.fs;
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.junit.After;
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Test;
 
 public class TestFileUtil {
+  private static final Log LOG = LogFactory.getLog(TestFileUtil.class);
+
   final static private File TEST_DIR = new File(System.getProperty(
       "test.build.data", "/tmp"), "fu");
-  static String FILE = "x";
-  static String LINK = "y";
-  static String DIR = "dir";
-  File del = new File(TEST_DIR, "del");
-  File tmp = new File(TEST_DIR, "tmp");
+  private static String FILE = "x";
+  private static String LINK = "y";
+  private static String DIR = "dir";
+  private File del = new File(TEST_DIR, "del");
+  private File tmp = new File(TEST_DIR, "tmp");
 
   /**
    * Creates directories del and tmp for testing.
@@ -47,8 +53,7 @@ public class TestFileUtil {
    *   link: y to tmp/x
    *   link: tmpDir to tmp
    */
-  @Before
-  public void setUp() throws IOException {
+  private void setupDirs() throws IOException {
     Assert.assertFalse(del.exists());
     Assert.assertFalse(tmp.exists());
     del.mkdirs();
@@ -83,14 +88,18 @@ public class TestFileUtil {
 
   @Test
   public void testFullyDelete() throws IOException {
-    FileUtil.fullyDelete(del);
+    setupDirs();
+    boolean ret = FileUtil.fullyDelete(del);
+    Assert.assertTrue(ret);
     Assert.assertFalse(del.exists());
     validateTmpDir();
   }
 
   @Test
   public void testFullyDeleteContents() throws IOException {
-    FileUtil.fullyDeleteContents(del);
+    setupDirs();
+    boolean ret = FileUtil.fullyDeleteContents(del);
+    Assert.assertTrue(ret);
     Assert.assertTrue(del.exists());
     Assert.assertEquals(0, del.listFiles().length);
     validateTmpDir();
@@ -101,4 +110,143 @@ public class TestFileUtil {
     Assert.assertEquals(1, tmp.listFiles().length);
     Assert.assertTrue(new File(tmp, FILE).exists());
   }
+
+  private File xSubDir = new File(del, "xsubdir");
+  private File ySubDir = new File(del, "ysubdir");
+  static String file1Name = "file1";
+  private File file2 = new File(xSubDir, "file2");
+  private File file3 = new File(ySubDir, "file3");
+  private File zlink = new File(del, "zlink");
+  
+  /**
+   * Creates a directory which can not be deleted completely.
+   * 
+   * Directory structure. The naming is important in that {@link MyFile}
+   * is used to return them in alphabetical order when listed.
+   * 
+   *                     del(+w)
+   *                       |
+   *    .---------------------------------------,
+   *    |            |              |           |
+   *  file1(!w)   xsubdir(-w)   ysubdir(+w)   zlink
+   *                 |              |
+   *               file2          file3
+   *
+   * @throws IOException
+   */
+  private void setupDirsAndNonWritablePermissions() throws IOException {
+    Assert.assertFalse("The directory del should not have existed!",
+        del.exists());
+    del.mkdirs();
+    new MyFile(del, file1Name).createNewFile();
+
+    // "file1" is non-deletable by default, see MyFile.delete().
+
+    xSubDir.mkdirs();
+    file2.createNewFile();
+    xSubDir.setWritable(false);
+    ySubDir.mkdirs();
+    file3.createNewFile();
+
+    Assert.assertFalse("The directory tmp should not have existed!",
+        tmp.exists());
+    tmp.mkdirs();
+    File tmpFile = new File(tmp, FILE);
+    tmpFile.createNewFile();
+    FileUtil.symLink(tmpFile.toString(), zlink.toString());
+  }
+  
+  // Validates the return value.
+  // Validates the existence of directory "xsubdir" and the file "file1"
+  // Sets writable permissions for the non-deleted dir "xsubdir" so that it can
+  // be deleted in tearDown().
+  private void validateAndSetWritablePermissions(boolean ret) {
+    xSubDir.setWritable(true);
+    Assert.assertFalse("The return value should have been false!", ret);
+    Assert.assertTrue("The file file1 should not have been deleted!",
+        new File(del, file1Name).exists());
+    Assert.assertTrue(
+        "The directory xsubdir should not have been deleted!",
+        xSubDir.exists());
+    Assert.assertTrue("The file file2 should not have been deleted!",
+        file2.exists());
+    Assert.assertFalse("The directory ysubdir should have been deleted!",
+        ySubDir.exists());
+    Assert.assertFalse("The link zlink should have been deleted!",
+        zlink.exists());
+  }
+
+  @Test
+  public void testFailFullyDelete() throws IOException {
+    LOG.info("Running test to verify failure of fullyDelete()");
+    setupDirsAndNonWritablePermissions();
+    boolean ret = FileUtil.fullyDelete(new MyFile(del));
+    validateAndSetWritablePermissions(ret);
+  }
+
+  /**
+   * Extend {@link File}. Same as {@link File} except for two things: (1) This
+   * treats file1Name as a very special file which is not delete-able
+   * irrespective of it's parent-dir's permissions, a peculiar file instance for
+   * testing. (2) It returns the files in alphabetically sorted order when
+   * listed.
+   * 
+   */
+  public static class MyFile extends File {
+
+    private static final long serialVersionUID = 1L;
+
+    public MyFile(File f) {
+      super(f.getAbsolutePath());
+    }
+
+    public MyFile(File parent, String child) {
+      super(parent, child);
+    }
+
+    /**
+     * Same as {@link File#delete()} except for file1Name which will never be
+     * deleted (hard-coded)
+     */
+    @Override
+    public boolean delete() {
+      LOG.info("Trying to delete myFile " + getAbsolutePath());
+      boolean bool = false;
+      if (getName().equals(file1Name)) {
+        bool = false;
+      } else {
+        bool = super.delete();
+      }
+      if (bool) {
+        LOG.info("Deleted " + getAbsolutePath() + " successfully");
+      } else {
+        LOG.info("Cannot delete " + getAbsolutePath());
+      }
+      return bool;
+    }
+
+    /**
+     * Return the list of files in an alphabetically sorted order
+     */
+    @Override
+    public File[] listFiles() {
+      File[] files = super.listFiles();
+      List<File> filesList = Arrays.asList(files);
+      Collections.sort(filesList);
+      File[] myFiles = new MyFile[files.length];
+      int i=0;
+      for(File f : filesList) {
+        myFiles[i++] = new MyFile(f);
+      }
+      return myFiles;
+    }
+  }
+
+  @Test
+  public void testFailFullyDeleteContents() throws IOException {
+    LOG.info("Running test to verify failure of fullyDeleteContents()");
+    setupDirsAndNonWritablePermissions();
+    boolean ret = FileUtil.fullyDeleteContents(new MyFile(del));
+    validateAndSetWritablePermissions(ret);
+  }
 }