[
https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16165527#comment-16165527
]
Miklos Szegedi commented on YARN-6623:
--------------------------------------
Thank you for the patch [~vvasudev] and for the reviews [~leftnoteasy] and
[~ebadger].
Sorry about the delay, I had a chance to look at the latest patch now. I have
some comments but before that, does the jira target 3.0-beta1?
{code}
84 printWriter.println(" " + entry.getKey() + "=" + StringUtils
85 .join(",", entry.getValue()));
{code}
writeCommandToTempFile: entry.getKey() can still contain an = in the latest
patch, which is an issue. It probably needs to be filtered in
addCommandArguments.
{code}
701 char *get_config_path(const char *argv0) {
702 char *executable_file = get_executable((char *) argv0);
703 if (!executable_file) {
704 fprintf(ERRORFILE, "realpath of executable: %s\n",
705 errno != 0 ? strerror(errno) : "unknown");
706 return NULL;
707 }
{code}
It is probably a good idea to check for {{executable_file\[0\] != 0}} as well
{code}
1150 size_t command_size = MIN(sysconf(_SC_ARG_MAX), 128*1024);
1151 char *buffer = alloc_and_clear_memory(command_size, sizeof(char));
1152 ret = get_docker_command(command_file, &CFG, buffer,
EXECUTOR_PATH_MAX);
{code}
The code passes in a different size than the actual size of the buffer.
{code}
157 inline void* alloc_and_clear_memory(size_t num, size_t size) {
158 void *ret = calloc(num, size);
159 if (ret == NULL) {
160 exit(OUT_OF_MEMORY);
161 }
162 return ret;
163 }
{code}
It might be a good idea to print an error message here.
{code}
42 static int add_to_buffer(char *buff, const size_t bufflen, const char
*string) {
{code}
Why do not you use strncat inside? It would spare one of the strlen’s.
{code}
105 if(prefix != 0) {
106 tmp_ptr = strchr(values[i], prefix);
107 if (tmp_ptr == NULL) {
...
{code}
This feels a little bit less readable. I would suggest having a len instead of
tmp_ptr defaulted to strlen(tmp_ptr); Also, am I right that we are checking
only if the left device is allowed?
{code}
150 if (ret != 0) {
151 memset(out, 0, outlen);
152 }
{code}
out\[0\]=0 should be sufficient, if outlen > 0 and ret != 0
{code}
162 if (0 == strncmp(container_name, CONTAINER_NAME_PREFIX,
strlen(CONTAINER_NAME_PREFIX))) {
{code}
There is no need of an strlen here, sizeof is sufficient and calculates compile
time
{code}
283 ret = add_docker_config_param(&command_config, out, outlen);
284 if (ret != 0) {
285 return BUFFER_TOO_SMALL;
{code}
Container name is not freed.
{code}
330 if (ret != 0) {
331 return BUFFER_TOO_SMALL;
332 }
{code}
Image name is not freed.
{code}
381 quote_and_append_arg(&tmp_buffer, &tmp_buffer_size, " ", image_name);
{code}
That space might need to be added to the quote_and_append_arg function for
safety reasons.
{code}
564 * 2. If the path is a directory, add a '/' at the end( if not present)
{code}
There is a small typo here.
{code}
585 if (len <= 0) {
586 return NULL;
587 }
{code}
There is a missing free here
{code}
731 strncpy(tmp_buffer_2, values[i], strlen(values[i]));
732 strncpy(tmp_buffer_2 + strlen(values[i]), ro_suffix, strlen(ro_suffix));
{code}
Why do you use strncpy here? Why not strcpy and strcat?
{code}
739 memset(tmp_buffer, 0, tmp_buffer_size);
{code}
Clearing the first character should be sufficient.
> Add support to turn off launching privileged containers in the
> container-executor
> ---------------------------------------------------------------------------------
>
> Key: YARN-6623
> URL: https://issues.apache.org/jira/browse/YARN-6623
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Priority: Blocker
> Attachments: YARN-6623.001.patch, YARN-6623.002.patch,
> YARN-6623.003.patch, YARN-6623.004.patch, YARN-6623.005.patch,
> YARN-6623.006.patch, YARN-6623.007.patch, YARN-6623.008.patch,
> YARN-6623.009.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should
> add a flag to the container-executor.cfg allowing admins to disable launching
> privileged containers at the container-executor level.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
