[
https://issues.apache.org/jira/browse/YARN-6852?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16112193#comment-16112193
]
Miklos Szegedi edited comment on YARN-6852 at 8/3/17 5:05 AM:
--------------------------------------------------------------
…
{{Return a parsed commandline options.}} There is a typo in this sentence.
{code}
struct section empty_executor_cfg = {.size=0, .kv_pairs=NULL};
{code}
This pattern is C++0x, should not be used in standard C. Note: I am not against
converting the whole tool to C++…
Moreover {{section->name}} is not set to NULL above.
{code}
const struct section* cfg_section;
static int config_initialized = 0;
{code}
I see the same issues as in the groups case. (cfg_section==NULL can be used
instead of config_initialized, cfg_section can be static, etc.)
n_minor_devices_to_block could be unsigned int, so that the negative check is
not needed
strtol is a better alternative to atoi
{code}
char param_value[128];
snprintf(param_value, 128, "c %d:%d rwm", major_device_number, i);
{code}
This could be written as:
{code}
snprintf(param_value, sizeof(param_value), "c %d:%d rwm",
major_device_number, i);
{code}
I do not see allowed_minor_numbers released anywhere.
{code}
char container_id[128];
memset(container_id, 0, 128);
{code}
It should be memset(container_id, 0, sizeof(container_id));
{{strcpy(container_id, optarg);}} This is dangerous without size. Use strncpy.
{{fflush(LOGFILE);}} This avoids caching and can be a performance bottleneck. I
think it is better to avoid unless there is a good reason.
{code}
const char *cgroups_param_path;
const char* cgroups_param_value;
{code}
Misaligned space.
In module_enabled I would name rc something else. You marked 0 as rc success in
other functions.
all_numbers: You touch the characters n^2 times. You should call strlen() once
and cache the value.
all_numbers:
{code}
if (strlen(input) == 0) {
return 0;
}
{code}
This is not necessary.
{code}
int* arr = (*numbers);
arr = malloc(sizeof(int) * n_numbers);
{code}
Does this return anything? I think it should be:
{code}
(*numbers) = malloc(sizeof(int) * n_numbers);
{code}
.
{code}
char* input_cpy = malloc(strlen(input));
strcpy(input_cpy, input);
{code}
There is no null pointer check.
{{arr[idx] = n;}} There is no overflow check. This could also be exploitable.
get_numbers_split_by_comma will return an array if a single 0 for an empty
string. It should return ret_n_number=0 instead.
{code}
if (strlen(p) == 0) {
return 0;
}
{code}
You could just check p[0]==0
{code}
if (mkdirs(TEST_ROOT, 0755) != 0) {
exit(1);
}
{code}
This needs some logging to show what happened.
{{fprintf(LOGFILE, "\nTesting %s\n", __func__);}} GTest prints out the function
name itself.
container_1 is an invalid container id in the unit tests. They will fail.
There is no indentation after {{namespace ContainerExecutor}}
{{static std::vector<const char*> cgroups_parameters_invoked;}} I think you
should consider std::string here. No need to malloc later
You do not clean up files in the unit tests, do you? Is there a reason?
was (Author: [email protected]):
…
{{Return a parsed commandline options.}} There is a typo in this sentence.
{code}
struct section empty_executor_cfg = {.size=0, .kv_pairs=NULL};
{code}
This pattern is C++0x, should not be used in standard C. Note: I am not against
converting the whole tool to C++…
Moreover {{section->name}} is not set to NULL above.
{code}
const struct section* cfg_section;
static int config_initialized = 0;
{code}
I see the same issues as in the groups case. (cfg_section==NULL can be used
instead of config_initialized, cfg_section can be static, etc.)
n_minor_devices_to_block could be unsigned int, so that the negative check is
not needed
strtol is a better alternative to atoi
{code}
char param_value[128];
snprintf(param_value, 128, "c %d:%d rwm", major_device_number, i);
{code}
This could be written as:
{code}
snprintf(param_value, sizeof(param_value), "c %d:%d rwm",
major_device_number, i);
{code}
I do not see allowed_minor_numbers released anywhere.
{code}
char container_id[128];
memset(container_id, 0, 128);
{code}
It should be memset(container_id, 0, sizeof(container_id));
{{strcpy(container_id, optarg);}} This is dangerous without size. Use strncpy.
{{fflush(LOGFILE);}} This avoids caching and can be a performance bottleneck. I
think it is better to avoid unless there is a good reason.
{code}
const char *cgroups_param_path;
const char* cgroups_param_value;
{code}
Misaligned space.
In module_enabled I would name rc something else. You marked 0 as rc success in
other functions.
all_numbers: You call strlen n^2 times. You should call it once and cache the
value.
all_numbers:
{code}
if (strlen(input) == 0) {
return 0;
}
{code}
This is not necessary.
{code}
int* arr = (*numbers);
arr = malloc(sizeof(int) * n_numbers);
{code}
Does this return anything? I think it should be:
{code}
(*numbers) = malloc(sizeof(int) * n_numbers);
{code}
.
{code}
char* input_cpy = malloc(strlen(input));
strcpy(input_cpy, input);
{code}
There is no null pointer check.
{{arr[idx] = n;}} There is no overflow check. This could also be exploitable.
get_numbers_split_by_comma will return an array if a single 0 for an empty
string. It should return ret_n_number=0 instead.
{code}
if (strlen(p) == 0) {
return 0;
}
{code}
You could just check p[0]==0
{code}
if (mkdirs(TEST_ROOT, 0755) != 0) {
exit(1);
}
{code}
This needs some logging to show what happened.
{{fprintf(LOGFILE, "\nTesting %s\n", __func__);}} GTest prints out the function
name itself.
container_1 is an invalid container id in the unit tests. They will fail.
There is no indentation after {{namespace ContainerExecutor}}
{{static std::vector<const char*> cgroups_parameters_invoked;}} I think you
should consider std::string here. No need to malloc later
You do not clean up files in the unit tests, do you? Is there a reason?
> [YARN-6223] Native code changes to support isolate GPU devices by using
> CGroups
> -------------------------------------------------------------------------------
>
> Key: YARN-6852
> URL: https://issues.apache.org/jira/browse/YARN-6852
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Wangda Tan
> Assignee: Wangda Tan
> Attachments: YARN-6852.001.patch, YARN-6852.002.patch
>
>
> This JIRA plan to add support of:
> 1) Isolation in CGroups. (native side).
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
