[
https://issues.apache.org/jira/browse/HADOOP-6818?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12920802#action_12920802
]
Erik Steffl commented on HADOOP-6818:
-------------------------------------
6818-trunk-1.patch review, nothing major, just few details to consider.
- line length over 80 chars, mismatched intendation in few cases (e.g.
JniBasedUnixGroupsMapping.java line: "be loaded");) etc., not sure how much we
care about detials like that
- TestJNIGroupsMapping.java - test compares groups returned by
ShellBasedUnixGroupsMapping and JniBasedUnixGroupsMapping, first it compares
group lists for size then it looks up whether all groups returned by
ShellBasedUnixGroupsMapping are also in JniBasedUnixGroupsMapping. This
comparison would fail for lists like (a, a, b) and (a, b, c). It's very
unlikely that ShellBasedUnixGroupsMapping would list the same group twice but
it's fairly cheap to do exact comparison (i.e. sort the lists and compare the
items one by one)
- JniBasedUnixGroupsMapping.java getGroups: is it not possible to return
empty array from getGroupForUser and get rid of if (groups != null &&
groups.length != 0) and the rest of the function?
- configure: what's the point of (unset CDPATH)? As far as I can tell it
creates a subshell in which it unsets CDPATH which has no effect in current
shell. I see the result is used to determine whether unset CDPATH is executed
but why? Given that it's not clear maybe a comment would help. (or is this file
generated?)
- JniBasedUnixGroupsMapping.c: it looks like code would be a bit simpler of
cleanup label did CHECK_ERROR and code that jumps to cleanup just set the
error. If cuser is NULL it could also jump to cleanup. That way there would be
only ene exit point of the function, one place responsible for resource cleanup
etc.
- getGroup.c: functions in this file do lot of pointer manipulation, it seems
they would also benefit from same techinzue used in
JniBasedUnixGroupsMapping.c, i.e. initialize all pointers to NULL, in case of
error jump to cleanup label that cleans everything up. Would free maintainer
from thinking about whether pwbuf or group (or both or neither) should be freed
or not etc.
- getGroup.c getGroupIDList: in case of error ngroup is not reset to zero,
shouldn't matter but would be more consistent (i.e. ngroups should always be
same as number of groups in groups)
> Provide a JNI-based implementation of GroupMappingServiceProvider
> -----------------------------------------------------------------
>
> Key: HADOOP-6818
> URL: https://issues.apache.org/jira/browse/HADOOP-6818
> Project: Hadoop Common
> Issue Type: Improvement
> Components: security
> Reporter: Devaraj Das
> Assignee: Devaraj Das
> Fix For: 0.22.0
>
> Attachments: 6818-trunk-1.patch, 6818-trunk.patch,
> hadoop-6818-1.patch, hadoop-6818-2.patch, JNIGroupMapping.patch
>
>
> The default implementation of GroupMappingServiceProvider does a fork of a
> unix command to get the groups of a user. Since the group resolution happens
> in the servers, this might be costly. This jira aims at providing a JNI-based
> implementation for GroupMappingServiceProvider.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
