From 7e599d9e3b852954a5a21b4738817c7aabfa1bc8 Mon Sep 17 00:00:00 2001 From: Aaron Myers Date: Wed, 9 Jan 2013 23:30:41 +0000 Subject: [PATCH] HADOOP-9155. FsPermission should have different default value, 777 for directory and 666 for file. Contributed by Binglin Chang. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1431148 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 3 +++ .../org/apache/hadoop/fs/FileContext.java | 24 +++++++++++++++--- .../java/org/apache/hadoop/fs/FileStatus.java | 13 +++++++--- .../java/org/apache/hadoop/fs/FileSystem.java | 6 ++--- .../apache/hadoop/fs/ftp/FTPFileSystem.java | 4 +-- .../apache/hadoop/fs/local/RawLocalFs.java | 2 +- .../hadoop/fs/permission/FsPermission.java | 25 ++++++++++++++++++- .../hadoop/fs/FileContextPermissionBase.java | 2 +- .../org/apache/hadoop/fs/TestFileStatus.java | 4 +-- .../TestLocalFSFileContextMainOperations.java | 12 +++++++++ .../fs/TestLocalFileSystemPermission.java | 2 +- .../org/apache/hadoop/hdfs/DFSClient.java | 8 +++--- .../hadoop/hdfs/protocol/HdfsFileStatus.java | 5 +++- 13 files changed, 88 insertions(+), 22 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index e10bab69c3b..618933b6c47 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -525,6 +525,9 @@ Release 2.0.3-alpha - Unreleased HADOOP-9181. Set daemon flag for HttpServer's QueuedThreadPool. (Liang Xie via suresh) + HADOOP-9155. FsPermission should have different default value, 777 for + directory and 666 for file. (Binglin Chang via atm) + Release 2.0.2-alpha - 2012-09-07 INCOMPATIBLE CHANGES diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java index 579c4e27498..978bb1ba0dd 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileContext.java @@ -172,7 +172,25 @@ public final class FileContext { public static final Log LOG = LogFactory.getLog(FileContext.class); + /** + * Default permission for directory and symlink + * In previous versions, this default permission was also used to + * create files, so files created end up with ugo+x permission. + * See HADOOP-9155 for detail. + * Two new constants are added to solve this, please use + * {@link FileContext#DIR_DEFAULT_PERM} for directory, and use + * {@link FileContext#FILE_DEFAULT_PERM} for file. + * This constant is kept for compatibility. + */ public static final FsPermission DEFAULT_PERM = FsPermission.getDefault(); + /** + * Default permission for directory + */ + public static final FsPermission DIR_DEFAULT_PERM = FsPermission.getDirDefault(); + /** + * Default permission for file + */ + public static final FsPermission FILE_DEFAULT_PERM = FsPermission.getFileDefault(); /** * Priority of the FileContext shutdown hook. @@ -656,7 +674,7 @@ public FSDataOutputStream create(final Path f, CreateOpts.Perms permOpt = (CreateOpts.Perms) CreateOpts.getOpt(CreateOpts.Perms.class, opts); FsPermission permission = (permOpt != null) ? permOpt.getValue() : - FsPermission.getDefault(); + FILE_DEFAULT_PERM; permission = permission.applyUMask(umask); final CreateOpts[] updatedOpts = @@ -704,7 +722,7 @@ public void mkdir(final Path dir, final FsPermission permission, IOException { final Path absDir = fixRelativePart(dir); final FsPermission absFerms = (permission == null ? - FsPermission.getDefault() : permission).applyUMask(umask); + FsPermission.getDirDefault() : permission).applyUMask(umask); new FSLinkResolver() { @Override public Void next(final AbstractFileSystem fs, final Path p) @@ -2157,7 +2175,7 @@ public boolean copy(final Path src, final Path dst, boolean deleteSource, FileStatus fs = FileContext.this.getFileStatus(qSrc); if (fs.isDirectory()) { checkDependencies(qSrc, qDst); - mkdir(qDst, FsPermission.getDefault(), true); + mkdir(qDst, FsPermission.getDirDefault(), true); FileStatus[] contents = listStatus(qSrc); for (FileStatus content : contents) { copy(makeQualified(content.getPath()), makeQualified(new Path(qDst, diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileStatus.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileStatus.java index ea2f1dc6169..c20f054d5e4 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileStatus.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileStatus.java @@ -79,8 +79,15 @@ public FileStatus(long length, boolean isdir, this.blocksize = blocksize; this.modification_time = modification_time; this.access_time = access_time; - this.permission = (permission == null) ? - FsPermission.getDefault() : permission; + if (permission != null) { + this.permission = permission; + } else if (isdir) { + this.permission = FsPermission.getDirDefault(); + } else if (symlink!=null) { + this.permission = FsPermission.getDefault(); + } else { + this.permission = FsPermission.getFileDefault(); + } this.owner = (owner == null) ? "" : owner; this.group = (group == null) ? "" : group; this.symlink = symlink; @@ -217,7 +224,7 @@ public void setPath(final Path p) { */ protected void setPermission(FsPermission permission) { this.permission = (permission == null) ? - FsPermission.getDefault() : permission; + FsPermission.getFileDefault() : permission; } /** diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java index 9c6e7df345b..9c1f2aaf73b 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java @@ -850,7 +850,7 @@ public FSDataOutputStream create(Path f, long blockSize, Progressable progress ) throws IOException { - return this.create(f, FsPermission.getDefault().applyUMask( + return this.create(f, FsPermission.getFileDefault().applyUMask( FsPermission.getUMask(getConf())), overwrite, bufferSize, replication, blockSize, progress); } @@ -1030,7 +1030,7 @@ public FSDataOutputStream createNonRecursive(Path f, boolean overwrite, int bufferSize, short replication, long blockSize, Progressable progress) throws IOException { - return this.createNonRecursive(f, FsPermission.getDefault(), + return this.createNonRecursive(f, FsPermission.getFileDefault(), overwrite, bufferSize, replication, blockSize, progress); } @@ -1866,7 +1866,7 @@ protected Path getInitialWorkingDirectory() { * Call {@link #mkdirs(Path, FsPermission)} with default permission. */ public boolean mkdirs(Path f) throws IOException { - return mkdirs(f, FsPermission.getDefault()); + return mkdirs(f, FsPermission.getDirDefault()); } /** diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java index d9b8ca63e1e..428bce3c9c6 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java @@ -224,7 +224,7 @@ public FSDataOutputStream create(Path file, FsPermission permission, } Path parent = absolute.getParent(); - if (parent == null || !mkdirs(client, parent, FsPermission.getDefault())) { + if (parent == null || !mkdirs(client, parent, FsPermission.getDirDefault())) { parent = (parent == null) ? new Path("/") : parent; disconnect(client); throw new IOException("create(): Mkdirs failed to create: " + parent); @@ -484,7 +484,7 @@ private boolean mkdirs(FTPClient client, Path file, FsPermission permission) if (!exists(client, absolute)) { Path parent = absolute.getParent(); created = (parent == null || mkdirs(client, parent, FsPermission - .getDefault())); + .getDirDefault())); if (created) { String parentDir = parent.toUri().getPath(); client.changeWorkingDirectory(parentDir); diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java index 9ce0a97ab13..85178f42d00 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java @@ -85,7 +85,7 @@ public void createSymlink(Path target, Path link, boolean createParent) "system: "+target.toString()); } if (createParent) { - mkdir(link.getParent(), FsPermission.getDefault(), true); + mkdir(link.getParent(), FsPermission.getDirDefault(), true); } // NB: Use createSymbolicLink in java.nio.file.Path once available try { diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java index a909edfd927..3db9acb2e22 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java @@ -275,11 +275,34 @@ public static void setUMask(Configuration conf, FsPermission umask) { conf.setInt(DEPRECATED_UMASK_LABEL, umask.toShort()); } - /** Get the default permission. */ + /** + * Get the default permission for directory and symlink. + * In previous versions, this default permission was also used to + * create files, so files created end up with ugo+x permission. + * See HADOOP-9155 for detail. + * Two new methods are added to solve this, please use + * {@link FsPermission#getDirDefault()} for directory, and use + * {@link FsPermission#getFileDefault()} for file. + * This method is kept for compatibility. + */ public static FsPermission getDefault() { return new FsPermission((short)00777); } + /** + * Get the default permission for directory. + */ + public static FsPermission getDirDefault() { + return new FsPermission((short)00777); + } + + /** + * Get the default permission for file. + */ + public static FsPermission getFileDefault() { + return new FsPermission((short)00666); + } + /** * Create a FsPermission from a Unix symbolic permission string * @param unixSymbolicPermission e.g. "-rw-rw-rw-" diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextPermissionBase.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextPermissionBase.java index b80764cebf0..2d7c687c6dd 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextPermissionBase.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileContextPermissionBase.java @@ -95,7 +95,7 @@ public void testCreatePermission() throws IOException { String filename = "foo"; Path f = getTestRootPath(fc, filename); createFile(fc, filename); - doFilePermissionCheck(FileContext.DEFAULT_PERM.applyUMask(fc.getUMask()), + doFilePermissionCheck(FileContext.FILE_DEFAULT_PERM.applyUMask(fc.getUMask()), fc.getFileStatus(f).getPermission()); } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileStatus.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileStatus.java index e5380484f97..5614dd6e56b 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileStatus.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileStatus.java @@ -121,7 +121,7 @@ public void constructorNoOwner() throws IOException { FileStatus fileStatus = new FileStatus(LENGTH, isdir, REPLICATION, BLKSIZE, MTIME, PATH); validateAccessors(fileStatus, LENGTH, isdir, REPLICATION, BLKSIZE, MTIME, - 0, FsPermission.getDefault(), "", "", null, PATH); + 0, FsPermission.getDirDefault(), "", "", null, PATH); } /** @@ -131,7 +131,7 @@ public void constructorNoOwner() throws IOException { public void constructorBlank() throws IOException { FileStatus fileStatus = new FileStatus(); validateAccessors(fileStatus, 0, false, 0, 0, 0, - 0, FsPermission.getDefault(), "", "", null, null); + 0, FsPermission.getFileDefault(), "", "", null, null); } /** diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFSFileContextMainOperations.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFSFileContextMainOperations.java index d1c272cc859..6f9248902eb 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFSFileContextMainOperations.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFSFileContextMainOperations.java @@ -24,6 +24,8 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.apache.hadoop.fs.FileContextTestHelper; +import org.apache.hadoop.fs.permission.FsPermission; public class TestLocalFSFileContextMainOperations extends FileContextMainOperationsBaseTest { @@ -47,4 +49,14 @@ public void testFileContextNoCache() throws UnsupportedFileSystemException { FileContext fc1 = FileContext.getLocalFSFileContext(); Assert.assertTrue(fc1 != fc); } + + @Test + public void testDefaultFilePermission() throws IOException { + Path file = FileContextTestHelper.getTestRootPath(fc, + "testDefaultFilePermission"); + FileContextTestHelper.createFile(fc, file); + FsPermission expect = FileContext.FILE_DEFAULT_PERM.applyUMask(fc.getUMask()); + Assert.assertEquals(expect, fc.getFileStatus(file) + .getPermission()); + } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystemPermission.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystemPermission.java index 4508e144a65..5e985737d3c 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystemPermission.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystemPermission.java @@ -73,7 +73,7 @@ public void testLocalFSsetPermission() throws IOException { try { FsPermission initialPermission = getPermission(localfs, f); System.out.println(filename + ": " + initialPermission); - assertEquals(FsPermission.getDefault().applyUMask(FsPermission.getUMask(conf)), initialPermission); + assertEquals(FsPermission.getFileDefault().applyUMask(FsPermission.getUMask(conf)), initialPermission); } catch(Exception e) { System.out.println(StringUtils.stringifyException(e)); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java index b0f80c780c3..9fa98fdd077 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java @@ -1157,7 +1157,7 @@ public OutputStream create(String src, boolean overwrite, short replication, /** * Call {@link #create(String, FsPermission, EnumSet, short, long, * Progressable, int, ChecksumOpt)} with default permission - * {@link FsPermission#getDefault()}. + * {@link FsPermission#getFileDefault()}. * * @param src File name * @param overwrite overwrite an existing file if true @@ -1175,7 +1175,7 @@ public OutputStream create(String src, Progressable progress, int buffersize) throws IOException { - return create(src, FsPermission.getDefault(), + return create(src, FsPermission.getFileDefault(), overwrite ? EnumSet.of(CreateFlag.CREATE, CreateFlag.OVERWRITE) : EnumSet.of(CreateFlag.CREATE), replication, blockSize, progress, buffersize, null); @@ -1206,7 +1206,7 @@ public DFSOutputStream create(String src, * * @param src File name * @param permission The permission of the directory being created. - * If null, use default permission {@link FsPermission#getDefault()} + * If null, use default permission {@link FsPermission#getFileDefault()} * @param flag indicates create a new file or create/overwrite an * existing file or append to an existing file * @param createParent create missing parent directory if true @@ -1232,7 +1232,7 @@ public DFSOutputStream create(String src, ChecksumOpt checksumOpt) throws IOException { checkOpen(); if (permission == null) { - permission = FsPermission.getDefault(); + permission = FsPermission.getFileDefault(); } FsPermission masked = permission.applyUMask(dfsClientConf.uMask); if(LOG.isDebugEnabled()) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsFileStatus.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsFileStatus.java index e0678672966..26b6e984d53 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsFileStatus.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsFileStatus.java @@ -67,7 +67,10 @@ public HdfsFileStatus(long length, boolean isdir, int block_replication, this.modification_time = modification_time; this.access_time = access_time; this.permission = (permission == null) ? - FsPermission.getDefault() : permission; + ((isdir || symlink!=null) ? + FsPermission.getDefault() : + FsPermission.getFileDefault()) : + permission; this.owner = (owner == null) ? "" : owner; this.group = (group == null) ? "" : group; this.symlink = symlink;