Author: bobby
Date: Thu Jan 17 19:19:10 2013
New Revision: 1434868
URL: http://svn.apache.org/viewvc?rev=1434868&view=rev
Log:
HADOOP-8849. FileUtil#fullyDelete should grant the target directories +rwx
permissions (Ivan A. Veselovsky via bobby)
Modified:
hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Modified: hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
URL:
http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt?rev=1434868&r1=1434867&r2=1434868&view=diff
==============================================================================
--- hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
(original)
+++ hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt Thu Jan
17 19:19:10 2013
@@ -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
Modified:
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
URL:
http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java?rev=1434868&r1=1434867&r2=1434868&view=diff
==============================================================================
---
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
(original)
+++
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
Thu Jan 17 19:19:10 2013
@@ -87,33 +87,98 @@ public class FileUtil {
* (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(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(File dir) {
+ 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 class FileUtil {
// 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
}
}
}
Modified:
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
URL:
http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java?rev=1434868&r1=1434867&r2=1434868&view=diff
==============================================================================
---
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
(original)
+++
hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Thu Jan 17 19:19:10 2013
@@ -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 class TestFileUtil {
//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 @@ public class TestFileUtil {
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 @@ public class TestFileUtil {
* |
* .---------------------------------------,
* | | | |
- * 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 @@ public class TestFileUtil {
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 @@ public class TestFileUtil {
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 class TestFileUtil {
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 class TestFileUtil {
*/
@Override
public File[] listFiles() {
- File[] files = super.listFiles();
+ final File[] files = super.listFiles();
+ if (files == null) {
+ return null;
+ }
List<File> filesList = Arrays.asList(files);
Collections.sort(filesList);
File[] myFiles = new MyFile[files.length];
@@ -405,10 +466,18 @@ public class TestFileUtil {
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();
boolean copyMergeResult = copyMerge("partitioned", "tmp/merged");
