On Tue, May 10, 2016 at 12:54:40AM +0200, maxime de Roucy wrote: > I forgot to free the memory allocated at 'filename = calloc' (why valgrind > didn't warn...). Forget this patch. I will send another one tomorow.
Yes I noticed, and there's this one as well : > > + wlt = calloc(1, sizeof(*wlt)); > > + tmp_str = strdup(filename); > > + if (!wlt || !tmp_str) { > > + Alert("Cannot load configuration > > files %s : out of memory.\n", > > + filename); > > + exit(1); > > + } If tmp_str fails and wlt succeeds, wlt is not freed. Overall I still think that writing this into a dedicated function will make the controls and unrolling easier. I'd suggest something like this which is much more conventional, auditable and maintainable : /* loads config file/dir <arg>, returns 0 on failure with errmsg * filled with an error message. */ int load_config_file(const char *arg, char **errmsg) { ... wlt = calloc(1, sizeof(*wlt)); if (!wlt) { memprintf(errmsg, "out of memory"); goto fail_wlt; } tmp_str = strdup(filename); if (!tmp_str) { memprintf(errmsg, "out of memory"); goto fail_tmp_str; } ... return 1; ... fail_smp_str: free(wlt); fail_wlt: ... return 0; } Then in init() : if (!load_config_from_file(argv, &errmsg)) { Alert("Cannot load configuration files %s : %s.\n", filename, errmsg); exit(1); } Also, please don't use sprintf() but snprintf() as I mentionned in the earlier mail. Some platforms will systematically emit a warning when sprintf() is used, for the same reason as strcpy() and strcat(), and we managed to get rid of the last one already : > > + struct dirent *dir_entry = dir_entries[dir_entries_it]; > > + char *filename; > > + int filename_size = strlen(wl->s) > > + + 1 /* for '/' */ > > + + strlen(dir_entry->d_name); > > + > > + filename = calloc(filename_size + 1 /* for \0 */, > > sizeof(*filename)); (...) > > + sprintf(filename, "%s/%s", wl->s, dir_entry->d_name); At first I thought there was a bug in the calloc call but it's OK. It's not much common to use it with anything but "1" in the element size so it confused me. You don't need to justify that +1 is for \0 since that's usual in every string allocation. Thanks, Willy