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
