Hi Willy, > > I don't receive all the mails from haproxy@formilux.org. > > For exemple I didn't received : > > http://article.gmane.org/gmane.comp.web.haproxy/27795 > > Well, this one was sent to you directly by Cyril, so you should have > received it.
Indeed, it was this one: http://article.gmane.org/gmane.comp.web.haproxy/27802 I will check my anti-spam. Anyway : > I've subscribed you by hand now. I now receive all the mailling mail. Thanks a lot. > > > 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. > > Yes I know but as I said, if/when such code later moves to its own > function, this function might initially decide to exit then to let > the caller take the decision and one day all of this will be used > dynamically or from the CLI and then people discover a memory leak. > And there are the valgrind users who send patches very often to fix > such warnings that annoy them. I mean we spent a lot of time killing > some such old issues that were not bugs initially and that became > bugs later, so we try to be careful. We don't want to be the next > openssl if you see what I mean :-) Yes I see :-) For the record I now always check my code with "valgrind --leak- check=full". > > 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). > > That's the best way to deal with error unrolling. I'm sad that > teachers at school teach students not to use it because : > 1) it's what the compiler implements anyway for all other > constructs > 2) it's the only safe way to perform unrolling which resists to > code additions. > > We used to have some leaks in the past because we were not using it. > When you have some session initialization code like this : > > s = malloc(sizeof(*s)); > if (!s) > return; > > s->req = malloc(sizeof(*s->req)); > if (!s->req)) { > free(s); > return; > } > > s->res = malloc(sizeof(*s->res)); > if (!s->res)) { > free(s->req); > free(s); > return; > } > > s->txn = malloc(sizeof(*s->txn)); > if (!s->txn)) { > free(s->res); > free(s->req); > free(s); > return; > } > > s->log = malloc(sizeof(*s->log)); > if (!s->log)) { > free(s->txn); > free(s->res); > free(s->req); > free(s); > return; > } > > s->req_capture = malloc(sizeof(*s->req_capture)); > if (!s->req_capture)) { > free(s->log); > free(s->txn); > free(s->res); > free(s->req); > free(s); > return; > } > > s->res_capture = malloc(sizeof(*s->res_capture)); > if (!s->res_capture)) { > free(s->req_capture); > free(s->log); > free(s->txn); > free(s->res); > free(s->req); > free(s); > return; > } > > ... and so on for 10 entries ... > > Then you may already have bugs above due to the inevitable copy- > paste, and once you insert a new field in the middle (eg: s->vars) > you're pretty sure that you will miss it in one of the next "if" > blocks because they are never as clear as above but themselves > enclosed within other "if" blocks. And when you need to switch > allocation order, that's even worse. But the horrible thing above can > be safely turned into this : > > s = malloc(sizeof(*s)); > if (!s) > goto fail_s; > > s->req = malloc(sizeof(*s->req)); > if (!s->req)) > goto fail_req; > > s->res = malloc(sizeof(*s->res)); > if (!s->res)) > goto fail_res; > > s->txn = malloc(sizeof(*s->txn)); > if (!s->txn)) > goto fail_txn; > > s->log = malloc(sizeof(*s->log)); > if (!s->log)) > goto fail_log; > > s->req_capture = malloc(sizeof(*s->req_capture)); > if (!s->req_capture)) > goto fail_req_cap; > > s->res_capture = malloc(sizeof(*s->res_capture)); > if (!s->res_capture)) > goto fail_res_cap; > > return s; > > fail_res_cap: > free(s->req_capture); > fail_req_cap: > free(s->log); > fail_log: > free(s->txn); > fail_txn: > free(s->res); > fail_res: > free(s->req); > fail_req: > free(s); > fail_s: > return NULL; > > And a nice side effect is that when you look at the assembly code, > it's much smaller and much more efficient. > > > It doesn't reduce the number of lines and, as after every alert we > > call exit, there is no need to clean anything. > > As I explained above I agree on this but code correctness and code > cleanness are two different things. We try to have a bit of > modularity because we know that code moves and that it's better if it > can move safely. For example I recently reimplemented all the stats > dump to be able to dump *all* the information we have. It was > something like 50 patches. I felt like this code had not changed for > 10 years and that it would not change for the 10 next years. One week > later, Thierry sent me a huge patch exploding all my functions > and moving them around in order to call them from Lua, something I > hadn't even imagined. Wow, that's an explanation, thanks. I tried to use it on the following patch. Did I used it correctly ? > I guess you'll find me boring or annoying initially, but > over time once you see other people change your code and avoid a few > traps, you'll find that such comments make sense :-) You are not boring at all :-) And so far you explained (with lot's of details) all the changed you asked me to make, so that's absolutely fine by me. > > @@ -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); > > } > > I could have missed something but I think that the only purpose of > this part is to decide whether or not you're going to free wl->s, am > I right ? > If so, why not simply free(wl->s) unconditionally ? I think I may be > missing something. Some "wl->s" were pointing to "argv*" elements so we can't free theme. In the following patch I decided to create a fonction (list_append_word) that copy it's string argument en append it to a list. Because it's a copy (in the heap) we now have to free every "wl->s" ; which also make things simpler. Regards Maxime de Roucy ############ diff --git a/include/common/standard.h b/include/common/standard.h index cd2208c..f123f1a 100644 --- a/include/common/standard.h +++ b/include/common/standard.h @@ -1089,4 +1089,12 @@ static inline unsigned long long rdtsc() } #endif +/* append a copy of string <str> (in a wordlist) at the end of the list <li> + * On failure : return 0 and <err> filled with an error message. + * The caller is responsible for freeing the <err> and <str> copy + * memory area using free() + */ +struct list; +int list_append_word(struct list *li, const char *str, char **err); + #endif /* _COMMON_STANDARD_H */ diff --git a/src/haproxy.c b/src/haproxy.c index 0c223e5..6e5eecc 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,87 @@ 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; + char *err = NULL; + + 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; + + /* from this point wl->s is a directory */ + + 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 each element in the directory wl->s */ + 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 = NULL; + + if (!memprintf(&filename, "%s/%s", wl->s, dir_entry->d_name)) { + Alert("Cannot load configuration files %s : out of memory.\n", + filename); + exit(1); + } + + 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)) { + if (!list_append_word(&cfg_cfgfiles, filename, &err)) { + Alert("Cannot load configuration files %s : %s\n", + filename, + err); + exit(1); + } + } + + free(filename); + free(dir_entry); + } + + free(dir_entries); + + /* remove the current directory (wl) from cfg_cfgfiles */ + free(wl->s); + LIST_DEL(&wl->list); + free(wl); + } + + free(err); +} + /* * This function initializes all the necessary variables. It only returns * if everything is OK. If something fails, it exits. @@ -561,6 +644,7 @@ void init(int argc, char **argv) char *tmp; char *cfg_pidfile = NULL; int err_code = 0; + char *err_msg = NULL; struct wordlist *wl; char *progname; char *change_dir = NULL; @@ -713,13 +797,12 @@ void init(int argc, char **argv) /* now that's a cfgfile list */ argv++; argc--; while (argc > 0) { - wl = calloc(1, sizeof(*wl)); - if (!wl) { - Alert("Cannot load configuration file %s : out of memory.\n", *argv); + if (!list_append_word(&cfg_cfgfiles, *argv, &err_msg)) { + Alert("Cannot load configuration file/directory %s : %s\n", + *argv, + err_msg); exit(1); } - wl->s = *argv; - LIST_ADDQ(&cfg_cfgfiles, &wl->list); argv++; argc--; } break; @@ -736,13 +819,12 @@ void init(int argc, char **argv) case 'N' : cfg_maxpconn = atol(*argv); break; case 'L' : strncpy(localpeer, *argv, sizeof(localpeer) - 1); break; case 'f' : - wl = calloc(1, sizeof(*wl)); - if (!wl) { - Alert("Cannot load configuration file %s : out of memory.\n", *argv); + if (!list_append_word(&cfg_cfgfiles, *argv, &err_msg)) { + Alert("Cannot load configuration file/directory %s : %s\n", + *argv, + err_msg); exit(1); } - wl->s = *argv; - LIST_ADDQ(&cfg_cfgfiles, &wl->list); break; case 'p' : cfg_pidfile = *argv; break; default: usage(progname); @@ -758,14 +840,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(); @@ -1160,6 +1245,8 @@ void init(int argc, char **argv) /* initialize structures for name resolution */ if (!dns_init_resolvers()) exit(1); + + free(err_msg); } static void deinit_acl_cond(struct acl_cond *cond) @@ -1550,6 +1637,7 @@ void deinit(void) free(log); } list_for_each_entry_safe(wl, wlb, &cfg_cfgfiles, list) { + free(wl->s); LIST_DEL(&wl->list); free(wl); } diff --git a/src/standard.c b/src/standard.c index a4d2097..0de29ea 100644 --- a/src/standard.c +++ b/src/standard.c @@ -3439,6 +3439,38 @@ unsigned char utf8_next(const char *s, int len, unsigned int *c) return code | ((p-(unsigned char *)s)&0x0f); } +/* append a copy of string <str> (in a wordlist) at the end of the list <li> + * On failure : return 0 and <err> filled with an error message. + * The caller is responsible for freeing the <err> and <str> copy + * memory area using free() + */ +int list_append_word(struct list *li, const char *str, char **err) +{ + struct wordlist *wl; + + wl = calloc(1, sizeof(*wl)); + if (!wl) { + memprintf(err, "out of memory"); + goto fail_wl; + } + + wl->s = strdup(str); + if (!wl->s) { + memprintf(err, "out of memory"); + goto fail_wl_s; + } + + LIST_ADDQ(li, &wl->list); + + return 1; + +fail_wl_s: + free(wl->s); +fail_wl: + free(wl); + return 0; +} + /* * Local variables: * c-indent-level: 8 -- 2.8.2
signature.asc
Description: This is a digitally signed message part
