[
https://issues.apache.org/jira/browse/HADOOP-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12659223#action_12659223
]
Hemanth Yamijala commented on HADOOP-4930:
------------------------------------------
This is looking good. But I do have some more comments.
- main seems to assume the same number of arguments for all calls to the
taskcontroller. While that is true for the two commands we implemented
currently, it might not be true in future. It may be better to move the
arguments processing per command.
- umask needs to be set before launching the executable. This is because the
child java process creates files that should have the right permissions.
- Most of the comments that remain are related to the configuration parsing. I
have two suggestions:
-- As a first option, if we can find a package that would do this parsing
automatically for us, and that package is easy to include with Hadoop (or
better still, already is or is widely available), then that would be great to
use, and we can just do away with all the complex parsing logic.
-- If not, at a minimum, let's move all configuration related code to a
separate file so that it can be managed easily.
- Some configuration related comments:
-- It seems good to have all state of configuration in a separate struct,
something like:
{code}
struct configuration {
int size;
struct confentry** confdetails;
};
{code}
-- get_value is returning nothing when there's an error in get_configs, where a
NULL needs to be returned.
-- the buffer length passed to the fgets call should actually be decremented by
the amount already read, since fgets will return on seeing a newline
immediately.
-- strncat does not specifically return any error related values. The check for
NULL seems unnecessary.
-- The config entries are not being freed correctly on error conditions.
Basically, free_configurations should be enhanced to handle partially allocated
entries.
-- The contents buffer should not be freed at the end of parsing, but just
before the exec call. This is because configuration is holding pointers to the
buffer as keys and values.
-- search_conf variable is unused.
-- It will be more conventional to have the length of strings being calculated
to have the exact count, and add a character for null termination only in the
malloc / realloc calls. This way, other calls like snprintf can use strlen
directly without having to worry about the null character.
-- It is probably also safer to memset allocated buffers with 0s.
> Implement setuid executable for Linux to assist in launching tasks as job
> owners
> --------------------------------------------------------------------------------
>
> Key: HADOOP-4930
> URL: https://issues.apache.org/jira/browse/HADOOP-4930
> Project: Hadoop Core
> Issue Type: Sub-task
> Components: mapred
> Reporter: Hemanth Yamijala
> Assignee: Sreekanth Ramakrishnan
> Attachments: HADOOP-4930-1.patch, HADOOP-4930-2.patch,
> HADOOP-4930-3.patch, HADOOP-4930-4.patch
>
>
> HADOOP-4490 tracks the design and implementation of launching tasks as job
> owners. As per discussion there, the proposal is to implement a setuid
> executable for Linux that would be launched by the tasktracker to run tasks
> as the job owners. In this task we will track the implementation of the
> setuid executable. HADOOP-4490 will be used to track changes in the
> tasktracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
