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
