[ https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16113478#comment-16113478 ]
Miklos Szegedi commented on YARN-6033: -------------------------------------- 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