I'm not seeing any signs of this feature sliding into 1.9 source - any danger of it not going in to the current dev branch? Are there further concerns/problems/... standing in the way ? (it addresses one of my few haproxy gripes)
...jfree [ grateful/impressed haproxy user - thanks to all involved ] On Fri, Apr 27, 2018 at 10:59 PM, Willy Tarreau <[email protected]> wrote: > On Fri, Apr 27, 2018 at 08:58:52PM -0600, Ben Draut wrote: > > > > newnameserver->addr = *sk; > > > > } > > > > + else if (strcmp(args[0], "parse-resolv-conf") == 0) { > > > > > > I think you should register a config keyword and parse this in its own > > > function if at all possible, but I don't know if resolvers can use > > > registered config keywords, so if it's not possible, please ignore this > > > comment. > > > > > > > The resolvers section isn't registering any config keywords at the > moment, > > so I'm going to leave it the way it is to be consistent. > > OK. > > > > > + free(sk); > > > > + free(resolv_line); > > > > + if (fclose(f) != 0) { > > > > + ha_warning("parsing [%s:%d] : failed to close > > > handle to /etc/resolv.conf.\n", > > > > + file, linenum); > > > > + err_code |= ERR_WARN; > > > > + } > > > > > > In practice you don't need to run this check on a read-only file, as it > > > cannot fail, and if it really did, the user couldn't do anything about > > > it anyway. > > > > > > > Great, removed. > > > > I also fixed the memory leaks that you pointed out. (I think) But I did > > notice that > > valgrind reports that the 'newnameserver' allocation is being leaked > > anyway, both > > when using parse-resolv-conf as well as the regular nameserver > > directive...Let > > me know if I should do something about that. To me it seems the resolvers > > code > > should be freeing that. > > It means there's nothing in the deinit() function to take care of the > nameservers. It would be better to do it just to avoid the warnings > you're seeing. Do not hesitate to propose a patch for this if you want, > and please mark it for backporting. > > > +resolv_out: > > + if (sk != NULL) > > + free(sk); > > Here you don't need the test because free(NULL) is a NOP. > > > + if (resolv_line != NULL) > > + free(resolv_line); > > Same here. > > If you want I can take care of them when merging. Let's wait for Baptiste's > ACK now. > > Thanks, > Willy > >
