Hello Willy, I don't receive all the mails from [email protected]. For exemple I didn't received : http://article.gmane.org/gmane.comp.web.haproxy/27795
However I am sure I sent a blank mail to [email protected] (I rechecked). Can you check on the server side ? > > 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. If tmp_str fails and wlt succeeds we still got the Alert and everything it freed on exit. Anyway the problem isn't here anymore as I get ride of strdup. See the end of this mail. > 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); > } I create the function "void cfgfiles_expand_directories(void)", but not the "load_config_file" one. I am not accustomed to using goto and it's hard for me to use it here as I actually don't see the point of it (in cfgfiles_expand_directories). It doesn't reduce the number of lines and, as after every alert we call exit, there is no need to clean anything. > Also, please don't use sprintf() but snprintf() as I mentionned in > the earlier mail. I used sprintf at first because the man says it's not C89 compatible : fprintf(), printf(), sprintf(), vprintf(), vfprintf(), vsprintf(): POSIX.1-2001, POSIX.1-2008, C89, C99. snprintf(), vsnprintf(): POSIX.1-2001, POSIX.1-2008, C99. and the CONTRIBUTING file says : It is important to keep in mind that certain facilities offered by recent versions must not be used in the code : - … - in general, anything which requires C99 (such as declaring variables in "for" statements) I changed it to snprintf, yet I didn't check the output as it shouldn't be able to fail (the string size == copied size == calloc size). Here is a new version of the patch. Only for the src/haproxy.c file. I will send a complete patch when everything will be settled. I also free head memory from the "wlt = calloc(1, sizeof(*wlt));" line, in the deinit function. I don't think it's necessary as deinit is followed by exit but I thought it would be cleaner as we also free cfg_cfgfiles elements. Thank you very much for all your comments ! Maxime de Roucy ##### diff --git a/src/haproxy.c b/src/haproxy.c index 0c223e5..3fae428 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -33,10 +33,12 @@ #include <ctype.h> #include <sys/time.h> #include <sys/types.h> +#include <sys/stat.h> #include <sys/socket.h> #include <netinet/tcp.h> #include <netinet/in.h> #include <arpa/inet.h> +#include <dirent.h> #include <netdb.h> #include <fcntl.h> #include <errno.h> @@ -423,7 +425,7 @@ void usage(char *name) { display_version(); fprintf(stderr, - "Usage : %s [-f <cfgfile>]* [ -vdV" + "Usage : %s [-f <cfgfile|cfgdir>]* [ -vdV" "D ] [ -n <maxconn> ] [ -N <maxpconn> ]\n" " [ -p <pidfile> ] [ -m <max megs> ] [ -C <dir> ] [-- <cfgfile>*]\n" " -v displays version ; -vv shows known build options.\n" @@ -551,6 +553,95 @@ void dump(struct sig_handler *sh) pool_gc2(); } +/* This function check if cfg_cfgfiles containes directories. + * If it find one, it add all the files (and only files) it containes + * in cfg_cfgfiles in place of the directory (and remove the directory). + * It add the files in lexical order. + */ +void cfgfiles_expand_directories(void) +{ + struct wordlist *wl, *wlb; + + list_for_each_entry_safe(wl, wlb, &cfg_cfgfiles, list) { + struct stat file_stat; + struct dirent **dir_entries; + int dir_entries_nb; + int dir_entries_it; + + if (stat(wl->s, &file_stat)) { + Alert("Cannot open configuration file/directory %s : %s\n", + wl->s, + strerror(errno)); + exit(1); + } + + if (!S_ISDIR(file_stat.st_mode)) + continue; + + dir_entries_nb = scandir(wl->s, &dir_entries, NULL, alphasort); + if (dir_entries_nb < 0) { + Alert("Cannot open configuration directory %s : %s\n", + wl->s, + strerror(errno)); + exit(1); + } + + for (dir_entries_it = 0; dir_entries_it < dir_entries_nb; dir_entries_it++) { + 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, sizeof(*filename)); + if (!filename) { + Alert("Cannot load configuration files %s : out of memory.\n", + filename); + exit(1); + } + + snprintf(filename, filename_size + 1, "%s/%s", wl->s, dir_entry->d_name); + + if (stat(filename, &file_stat)) { + Alert("Cannot open configuration file %s : %s\n", + wl->s, + strerror(errno)); + exit(1); + } + + /* don't add anything else than regular file in cfg_cfgfiles + * this way we avoid loops + */ + if (S_ISREG(file_stat.st_mode)) { + struct wordlist *wlt; + + wlt = calloc(1, sizeof(*wlt)); + if (!wlt) { + Alert("Cannot load configuration files %s : out of memory.\n", + filename); + exit(1); + } + + wlt->s = filename; + + /* add wlt (a file) before wl (a directory) + * in cfg_cfgfiles + */ + LIST_ADDQ(&wl->list, &wlt->list); + } + else { + free(filename); + } + free(dir_entry); + } + free(dir_entries); + + /* remove the current directory (wl) from cfg_cfgfiles */ + LIST_DEL(&wl->list); + free(wl); + } +} + /* * This function initializes all the necessary variables. It only returns * if everything is OK. If something fails, it exits. @@ -738,7 +829,7 @@ void init(int argc, char **argv) case 'f' : wl = calloc(1, sizeof(*wl)); if (!wl) { - Alert("Cannot load configuration file %s : out of memory.\n", *argv); + Alert("Cannot load configuration file/directory %s : out of memory.\n", *argv); exit(1); } wl->s = *argv; @@ -758,14 +849,17 @@ void init(int argc, char **argv) (arg_mode & (MODE_DAEMON | MODE_SYSTEMD | MODE_FOREGROUND | MODE_VERBOSE | MODE_QUIET | MODE_CHECK | MODE_DEBUG)); - if (LIST_ISEMPTY(&cfg_cfgfiles)) - usage(progname); - if (change_dir && chdir(change_dir) < 0) { Alert("Could not change to directory %s : %s\n", change_dir, strerror(errno)); exit(1); } + /* handle cfgfiles that are actualy directories */ + cfgfiles_expand_directories(); + + if (LIST_ISEMPTY(&cfg_cfgfiles)) + usage(progname); + global.maxsock = 10; /* reserve 10 fds ; will be incremented by socket eaters */ init_default_instance(); @@ -1238,7 +1332,7 @@ static void deinit_stick_rules(struct list *rules) } } -void deinit(void) +void deinit(int argc, char **argv) { struct proxy *p = proxy, *p0; struct cap_hdr *h,*h_next; @@ -1550,6 +1644,20 @@ void deinit(void) free(log); } list_for_each_entry_safe(wl, wlb, &cfg_cfgfiles, list) { + int argc_i = argc - 1; + char **argv_i = argv + 1; + int is_arg = 0; + + while (argc_i > 0) { + if (*argv_i == wl->s) + is_arg = 1; + argv_i++; + argc_i--; + } + + if (!is_arg) + free(wl->s); + LIST_DEL(&wl->list); free(wl); } @@ -1952,7 +2060,7 @@ int main(int argc, char **argv) run_poll_loop(); /* Do some cleanup */ - deinit(); + deinit(argc, argv); exit(0); } -- 2.8.2
signature.asc
Description: This is a digitally signed message part
