[
https://issues.apache.org/jira/browse/HDFS-5802?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14791589#comment-14791589
]
Yongjun Zhang commented on HDFS-5802:
-------------------------------------
HI [~xiaochen],
Thanks for working on this. The patch looks generally good. I have some
comments below, largely cosmetic.
1. Right now you changed {{private void checkTraverse(INodeAttributes[] inodes,
String path, int last)}} by adding a try/catch block, that catches the
exception then check if any ancestor is not directory, and
construct the path with {{InodeAttributes.toString()}}. This
{{InodeAttributes.toString()}} counts on that {{InodeAttributes}} is {{Inode}}
here, which is generally true but may not always be.
I think a better approach is to move this try/catch block to one layer above to
the caller {{checkPermission(...)}}, do something like
{code}
try {
checkTraverse(inodeAttrs, path, ancestorIndex);
} catch (AccessControlException e) {
checkAncestorType(inodes, ancestorIndex, e);
}
{code}
where is an Inode array {{inodes}} available, so we can use
Inode.getFullPathName()}} method. Basically move your code to a new method
{{checkAncestorType(inodes, ancestorIndex, e);}} above.
The loop starts with index 1 in your code, we can start with 0.
In the loop, we check its type when inodes[j] is not null; when it's null, we
should simply break and throw the original exception.
2. The message can be formatted a bit more like
{code}
throw new AccessControlException(e.getMessage() + " (Ancestor " +
inodes[j].getFullPathName() + " is not a directory).");
{code}
3. Suggest to rename {{testPermissionMessageOnNonexistingFile()}} to
{{testPermissionMessageOnNonDirAncestor()}}
4. split {{@Override public FileSystem run() throws Exception}} into two lines
so {{override}} is on its own line.
5. Couple of lines in the testing code exceeded 80 chars.
Thanks.
> NameNode does not check for inode type before traversing down a path
> --------------------------------------------------------------------
>
> Key: HDFS-5802
> URL: https://issues.apache.org/jira/browse/HDFS-5802
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: namenode
> Affects Versions: 2.0.0-alpha
> Reporter: Harsh J
> Assignee: Xiao Chen
> Priority: Trivial
> Attachments: HDFS-5802.001.patch
>
>
> This came up during the discussion on a forum at
> http://community.cloudera.com/t5/Batch-Processing-and-Workflow/Permission-denied-access-EXECUTE-on-getting-the-status-of-a-file/m-p/5049#M162
> surrounding an fs.exists(…) check running on a path /foo/bar, where /foo is
> a file and not a directory.
> In such a case, NameNode yields a user-confusing message of {{Permission
> denied: user=foo, access=EXECUTE, inode="/foo":foo:foo:-rw-r--r--}} instead
> of clearly saying (and realising) "/foo is not a directory" or "/foo is a
> file" before it tries to traverse further down to locate the requested path.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
