[
https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16113478#comment-16113478
]
Miklos Szegedi edited comment on YARN-6033 at 8/3/17 9:19 PM:
--------------------------------------------------------------
Thank you, [~vvasudev], [~sunilg] and [~wangda]. I looked through the latest
patch and found the following issues. I see one important below, the others
seem to be benign or style comments only.
free_section: Note: It is a good practice to set all released pointers to NULL.
{code}
51 if (section->size > 0) {
52 free(section->kv_pairs);
53 }
{code}
Note: Using a condition {{section->kv_pairs != NULL}} is safer.
{{char *dir = strdup(file_name);}} Note: It does not check for a NULL return
{{size_t linesize = 1000;}} Note: This may be too small for Docker whitelist.
{{if (strlen(line) == 0) {}} Note: …or just line[0]==0 BTW you use it again in
the same function walking through the string again.
{code}
278 len = strlen(line);
279 buffer = (char *) calloc(len + 1, sizeof(char));
280 strncpy(buffer, line, len);
{code}
How about strdup?
{code}
300 if (equaltok == NULL) {
301 // this can happen because no value was set
302 // e.g. banned.users=#this is a comment
303 if (strstr(line, splitter) == NULL) {
304 fprintf(ERRORFILE, "configuration tokenization failed, error with
line %s\n", line);
305 return -1;
306 }
307 }
{code}
Should not we free some memory before {{return -1;}}?
{code}
442 if (free_second_section) {
443 free(section2->name);
444 free(section2);
445 }
{code}
Important: Probably is a good idea to do a memset(section2, 0,
sizeof(section2)); here to avoid some debugging nightmare. The pointers point
to freed memory but valid data for now (which can be allocated and rewritten by
some junk later on) and kv pairs attached to another section, so would not
another run of {{get_configuration_section}} pick them up?
{code}
464 if (conf_file == NULL) {
465 fprintf(ERRORFILE, "Invalid conf file provided, unable to open file"
466 " : %s \n", file_path);
467 return (INVALID_CONFIG_FILE);
468 }
{code}
Note: cfg_sections is not freed, if the open fails. I suggest allocating any
memory after this passes.
{code}
cfg->sections[cfg->size]->name =
473 (char *) calloc(strlen("") + 1, sizeof(char));
474 strncpy(cfg->sections[cfg->size]->name, "", strlen(""));
{code}
Note: You can write this, or just {{cfg->sections[cfg->size]->name =
strdup("");}}
{code}
497 if ((cfg->size + 1) % MAX_SIZE == 0) {
498 cfg->sections = (struct section **) realloc(cfg->sections,
499 sizeof(struct sections *) * (MAX_SIZE +
cfg->size));
500 if (cfg->sections == NULL) {
501 fprintf(ERRORFILE,
502 "Failed re-allocating memory for configuration items\n");
503 exit(OUT_OF_MEMORY);
265 } 504 }
266 } 505 }
{code}
Note: This may leak memory. It should only be done if
{{cfg->sections[cfg->size]}}
was (Author: miklos.szeg...@cloudera.com):
Thank you, [~vvasudev] and [~wangda]. I looked through the latest patch and
found the following issues. I see one important below, the others seem to be
benign or style comments only.
free_section: Note: It is a good practice to set all released pointers to NULL.
{code}
51 if (section->size > 0) {
52 free(section->kv_pairs);
53 }
{code}
Note: Using a condition {{section->kv_pairs != NULL}} is safer.
{{char *dir = strdup(file_name);}} Note: It does not check for a NULL return
{{size_t linesize = 1000;}} Note: This may be too small for Docker whitelist.
{{if (strlen(line) == 0) {}} Note: …or just line[0]==0 BTW you use it again in
the same function walking through the string again.
{code}
278 len = strlen(line);
279 buffer = (char *) calloc(len + 1, sizeof(char));
280 strncpy(buffer, line, len);
{code}
How about strdup?
{code}
300 if (equaltok == NULL) {
301 // this can happen because no value was set
302 // e.g. banned.users=#this is a comment
303 if (strstr(line, splitter) == NULL) {
304 fprintf(ERRORFILE, "configuration tokenization failed, error with
line %s\n", line);
305 return -1;
306 }
307 }
{code}
Should not we free some memory before {{return -1;}}?
{code}
442 if (free_second_section) {
443 free(section2->name);
444 free(section2);
445 }
{code}
Important: Probably is a good idea to do a memset(section2, 0,
sizeof(section2)); here to avoid some debugging nightmare. The pointers point
to freed memory but valid data for now (which can be allocated and rewritten by
some junk later on) and kv pairs attached to another section, so would not
another run of {{get_configuration_section}} pick them up?
{code}
464 if (conf_file == NULL) {
465 fprintf(ERRORFILE, "Invalid conf file provided, unable to open file"
466 " : %s \n", file_path);
467 return (INVALID_CONFIG_FILE);
468 }
{code}
Note: cfg_sections is not freed, if the open fails. I suggest allocating any
memory after this passes.
{code}
cfg->sections[cfg->size]->name =
473 (char *) calloc(strlen("") + 1, sizeof(char));
474 strncpy(cfg->sections[cfg->size]->name, "", strlen(""));
{code}
Note: You can write this, or just {{cfg->sections[cfg->size]->name =
strdup("");}}
{code}
497 if ((cfg->size + 1) % MAX_SIZE == 0) {
498 cfg->sections = (struct section **) realloc(cfg->sections,
499 sizeof(struct sections *) * (MAX_SIZE +
cfg->size));
500 if (cfg->sections == NULL) {
501 fprintf(ERRORFILE,
502 "Failed re-allocating memory for configuration items\n");
503 exit(OUT_OF_MEMORY);
265 } 504 }
266 } 505 }
{code}
Note: This may leak memory. It should only be done if
{{cfg->sections[cfg->size]}}
> Add support for sections in container-executor configuration file
> -----------------------------------------------------------------
>
> Key: YARN-6033
> URL: https://issues.apache.org/jira/browse/YARN-6033
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Attachments: YARN-6033.003.patch, YARN-6033.004.patch,
> YARN-6033.005.patch, YARN-6033.006.patch, YARN-6033.007.patch,
> YARN-6033-YARN-5673.001.patch, YARN-6033-YARN-5673.002.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
