Hi Maxime, On Wed, May 11, 2016 at 12:37:36AM +0200, Maxime de Roucy wrote: > Hello 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. > However I am sure I sent a blank mail to haproxy+subscr...@formilux.org > (I rechecked). > Can you check on the server side ? Indeed you're not subscribed here. You might have some anti-spam filtering which blocked the validation e-mail. Some people have already had trouble with gmail's spam filtering. It seems to vary in time, but we have a bit more than 300 subscribers using gmail here so I guess overall it works. Ah and there's this well-known rant from Linus about a huge amount of false positives, lots of people are experiencing two-digit percents of false positive rate : https://plus.google.com/+LinusTorvalds/posts/DiG9qANf5PA I've subscribed you by hand now. > > > 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. 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 :-) > Anyway the problem isn't here anymore as I get ride of strdup. > See the end of this mail. OK. > 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. > > 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. Not exactly, snprintf() isn't related to the language but to the C library since it's a function and not a keyword, operator or language construct. However some language version mandate its implementation, so in theory in C99 you're supposed to find it (if you have a C library, because on bare metal you won't have it). What really matters in general is what a project uses because you want to ensure that your contribution will not be blamed for a build regression : $ git grep -w snprintf include/ src/|wc -l 92 $ git grep -w sprintf include/ src/ only reports outdated comments, such as : src/sample.c: char _str[7]; /* \u + 4 hex digit + null char for sprintf. */ > 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 see your point and I'm happy to see that there are some people who read this file, thank you for this :-) > 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). I'm perfectly fine with this. That's the exact purpose of the C language, it's a portable assembly code, the developer is supposed to be trusted. > 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. Thank you. 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 :-) > 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. Yes, the deinit function was made to make valgrind happy. I absolutely hate it and the existence of this function has significantly complicated the error unrolling in some initialization path (such as ssl) to the point of introducing several bugs (since after a partial failure it's very tricky to leave a partially initialized struct live till deinit()). At some point I wanted to kill it. But we managed to work around it and to make it work fine for the normal code path and to at least ensure it would not perform some double free on the most complex situations, so for now I'm fine with keeping it. And it forces us to leave things in a correct shape, that makes it easier to reuse parts of the initialization code for the CLI for example. I have a comment by the way : > @@ -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. Thanks! Willy
