diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 69ee7e9a90..8bbb52340c 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -1264,6 +1264,9 @@ Release 0.23.7 - UNRELEASED IMPROVEMENTS + HADOOP-8849. FileUtil#fullyDelete should grant the target directories +rwx + permissions (Ivan A. Veselovsky via bobby) + OPTIMIZATIONS BUG FIXES diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java index b6a2acae49..4593eedb9f 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java @@ -87,33 +87,98 @@ public static Path[] stat2Paths(FileStatus[] stats, Path path) { * (4) If dir is a normal directory, then dir and all its contents recursively * are deleted. */ - public static boolean fullyDelete(File dir) { - if (dir.delete()) { + public static boolean fullyDelete(final File dir) { + return fullyDelete(dir, false); + } + + /** + * Delete a directory and all its contents. If + * we return false, the directory may be partially-deleted. + * (1) If dir is symlink to a file, the symlink is deleted. The file pointed + * to by the symlink is not deleted. + * (2) If dir is symlink to a directory, symlink is deleted. The directory + * pointed to by symlink is not deleted. + * (3) If dir is a normal file, it is deleted. + * (4) If dir is a normal directory, then dir and all its contents recursively + * are deleted. + * @param dir the file or directory to be deleted + * @param tryGrantPermissions true if permissions should be modified to delete a file. + * @return true on success false on failure. + */ + public static boolean fullyDelete(final File dir, boolean tryGrantPermissions) { + if (tryGrantPermissions) { + // try to chmod +rwx the parent folder of the 'dir': + File parent = dir.getParentFile(); + grantPermissions(parent); + } + if (deleteImpl(dir, false)) { // dir is (a) normal file, (b) symlink to a file, (c) empty directory or // (d) symlink to a directory return true; } - // handle nonempty directory deletion - if (!fullyDeleteContents(dir)) { + if (!fullyDeleteContents(dir, tryGrantPermissions)) { return false; } - return dir.delete(); + return deleteImpl(dir, true); + } + + /* + * Pure-Java implementation of "chmod +rwx f". + */ + private static void grantPermissions(final File f) { + f.setExecutable(true); + f.setReadable(true); + f.setWritable(true); } + private static boolean deleteImpl(final File f, final boolean doLog) { + if (f == null) { + LOG.warn("null file argument."); + return false; + } + final boolean wasDeleted = f.delete(); + if (wasDeleted) { + return true; + } + final boolean ex = f.exists(); + if (doLog && ex) { + LOG.warn("Failed to delete file or dir [" + + f.getAbsolutePath() + "]: it still exists."); + } + return !ex; + } + /** * Delete the contents of a directory, not the directory itself. If * we return false, the directory may be partially-deleted. * If dir is a symlink to a directory, all the contents of the actual * directory pointed to by dir will be deleted. */ - public static boolean fullyDeleteContents(File dir) { + public static boolean fullyDeleteContents(final File dir) { + return fullyDeleteContents(dir, false); + } + + /** + * Delete the contents of a directory, not the directory itself. If + * we return false, the directory may be partially-deleted. + * If dir is a symlink to a directory, all the contents of the actual + * directory pointed to by dir will be deleted. + * @param tryGrantPermissions if 'true', try grant +rwx permissions to this + * and all the underlying directories before trying to delete their contents. + */ + public static boolean fullyDeleteContents(final File dir, final boolean tryGrantPermissions) { + if (tryGrantPermissions) { + // to be able to list the dir and delete files from it + // we must grant the dir rwx permissions: + grantPermissions(dir); + } boolean deletionSucceeded = true; - File contents[] = dir.listFiles(); + final File[] contents = dir.listFiles(); if (contents != null) { for (int i = 0; i < contents.length; i++) { if (contents[i].isFile()) { - if (!contents[i].delete()) {// normal file or symlink to another file + if (!deleteImpl(contents[i], true)) {// normal file or symlink to another file deletionSucceeded = false; continue; // continue deletion of other files/dirs under dir } @@ -121,16 +186,16 @@ public static boolean fullyDeleteContents(File dir) { // Either directory or symlink to another directory. // Try deleting the directory as this might be a symlink boolean b = false; - b = contents[i].delete(); + b = deleteImpl(contents[i], false); if (b){ //this was indeed a symlink or an empty directory continue; } // if not an empty directory or symlink let // fullydelete handle it. - if (!fullyDelete(contents[i])) { + if (!fullyDelete(contents[i], tryGrantPermissions)) { deletionSucceeded = false; - continue; // continue deletion of other files/dirs under dir + // continue deletion of other files/dirs under dir } } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 90db2d0526..a64b45d80f 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.fs; +import org.junit.Before; import java.io.BufferedReader; import java.io.File; import java.io.FileReader; @@ -173,12 +174,26 @@ public void testListAPI() throws IOException { //Expected an IOException } } + + @Before + public void before() throws IOException { + cleanupImpl(); + } @After public void tearDown() throws IOException { - FileUtil.fullyDelete(del); - FileUtil.fullyDelete(tmp); - FileUtil.fullyDelete(partitioned); + cleanupImpl(); + } + + private void cleanupImpl() throws IOException { + FileUtil.fullyDelete(del, true); + Assert.assertTrue(!del.exists()); + + FileUtil.fullyDelete(tmp, true); + Assert.assertTrue(!tmp.exists()); + + FileUtil.fullyDelete(partitioned, true); + Assert.assertTrue(!partitioned.exists()); } @Test @@ -269,12 +284,14 @@ private void validateTmpDir() { 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"); + private final File xSubDir = new File(del, "xSubDir"); + private final File xSubSubDir = new File(xSubDir, "xSubSubDir"); + private final File ySubDir = new File(del, "ySubDir"); + private static final String file1Name = "file1"; + private final File file2 = new File(xSubDir, "file2"); + private final File file22 = new File(xSubSubDir, "file22"); + private final File file3 = new File(ySubDir, "file3"); + private final File zlink = new File(del, "zlink"); /** * Creates a directory which can not be deleted completely. @@ -286,10 +303,14 @@ private void validateTmpDir() { * | * .---------------------------------------, * | | | | - * file1(!w) xsubdir(-w) ysubdir(+w) zlink - * | | - * file2 file3 - * + * file1(!w) xSubDir(-rwx) ySubDir(+w) zlink + * | | | + * | file2(-rwx) file3 + * | + * xSubSubDir(-rwx) + * | + * file22(-rwx) + * * @throws IOException */ private void setupDirsAndNonWritablePermissions() throws IOException { @@ -302,7 +323,16 @@ private void setupDirsAndNonWritablePermissions() throws IOException { xSubDir.mkdirs(); file2.createNewFile(); - xSubDir.setWritable(false); + + xSubSubDir.mkdirs(); + file22.createNewFile(); + + revokePermissions(file22); + revokePermissions(xSubSubDir); + + revokePermissions(file2); + revokePermissions(xSubDir); + ySubDir.mkdirs(); file3.createNewFile(); @@ -314,23 +344,43 @@ private void setupDirsAndNonWritablePermissions() throws IOException { FileUtil.symLink(tmpFile.toString(), zlink.toString()); } + private static void grantPermissions(final File f) { + f.setReadable(true); + f.setWritable(true); + f.setExecutable(true); + } + + private static void revokePermissions(final File f) { + f.setWritable(false); + f.setExecutable(false); + f.setReadable(false); + } + // 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!", + // Validates the existence of the file "file1" + private void validateAndSetWritablePermissions( + final boolean expectedRevokedPermissionDirsExist, final boolean ret) { + grantPermissions(xSubDir); + grantPermissions(xSubSubDir); + + 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!", + + Assert.assertEquals( + "The directory xSubDir *should* not have been deleted.", + expectedRevokedPermissionDirsExist, xSubDir.exists()); + Assert.assertEquals("The file file2 *should* not have been deleted.", + expectedRevokedPermissionDirsExist, file2.exists()); + Assert.assertEquals( + "The directory xSubSubDir *should* not have been deleted.", + expectedRevokedPermissionDirsExist, xSubSubDir.exists()); + Assert.assertEquals("The file file22 *should* not have been deleted.", + expectedRevokedPermissionDirsExist, file22.exists()); + + Assert.assertFalse("The directory ySubDir should have been deleted.", ySubDir.exists()); - Assert.assertFalse("The link zlink should have been deleted!", + Assert.assertFalse("The link zlink should have been deleted.", zlink.exists()); } @@ -339,7 +389,15 @@ public void testFailFullyDelete() throws IOException { LOG.info("Running test to verify failure of fullyDelete()"); setupDirsAndNonWritablePermissions(); boolean ret = FileUtil.fullyDelete(new MyFile(del)); - validateAndSetWritablePermissions(ret); + validateAndSetWritablePermissions(true, ret); + } + + @Test + public void testFailFullyDeleteGrantPermissions() throws IOException { + setupDirsAndNonWritablePermissions(); + boolean ret = FileUtil.fullyDelete(new MyFile(del), true); + // this time the directories with revoked permissions *should* be deleted: + validateAndSetWritablePermissions(false, ret); } /** @@ -388,7 +446,10 @@ public boolean delete() { */ @Override public File[] listFiles() { - File[] files = super.listFiles(); + final File[] files = super.listFiles(); + if (files == null) { + return null; + } List filesList = Arrays.asList(files); Collections.sort(filesList); File[] myFiles = new MyFile[files.length]; @@ -405,9 +466,17 @@ public void testFailFullyDeleteContents() throws IOException { LOG.info("Running test to verify failure of fullyDeleteContents()"); setupDirsAndNonWritablePermissions(); boolean ret = FileUtil.fullyDeleteContents(new MyFile(del)); - validateAndSetWritablePermissions(ret); + validateAndSetWritablePermissions(true, ret); } + @Test + public void testFailFullyDeleteContentsGrantPermissions() throws IOException { + setupDirsAndNonWritablePermissions(); + boolean ret = FileUtil.fullyDeleteContents(new MyFile(del), true); + // this time the directories with revoked permissions *should* be deleted: + validateAndSetWritablePermissions(false, ret); + } + @Test public void testCopyMergeSingleDirectory() throws IOException { setupDirs();