[CCing Alin for his opinion on Windows issue]
On Mon, Nov 05, 2018 at 04:20:18PM -0800, Yifeng Sun wrote:
> DNS resolution should fail if no DNS servers are available. This
> patch fixes it and also enables users to use environment variable
> OVS_RESOLV_CONF to specify the path for DNS server configuration
> file.
>
> Suggested-by: Ben Pfaff <b...@ovn.org>
> Suggested-by: Mark Michelson <mmich...@redhat.com>
> Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com>
> ---
> lib/dns-resolve.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dns-resolve.c b/lib/dns-resolve.c
> index 3c6d70e8fbba..60757cb1eb8a 100644
> --- a/lib/dns-resolve.c
> +++ b/lib/dns-resolve.c
> @@ -83,14 +83,26 @@ dns_resolve_init(bool is_daemon)
> }
>
> #ifdef __linux__
> - const char *filename = "/etc/resolv.conf";
> + const char *filename = getenv("OVS_RESOLV_CONF");
> + if (filename == NULL) {
> + filename = "/etc/resolv.conf";
> + }
> struct stat s;
> if (!stat(filename, &s) || errno != ENOENT) {
> int retval = ub_ctx_resolvconf(ub_ctx__, filename);
> if (retval != 0) {
> VLOG_WARN_RL(&rl, "Failed to read %s: %s",
> filename, ub_strerror(retval));
> + ub_ctx_delete(ub_ctx__);
> + ub_ctx__ = NULL;
> + return;
> }
> + } else {
> + VLOG_WARN_RL(&rl, "Failed to read %s: %s",
> + filename, ovs_strerror(errno));
> + ub_ctx_delete(ub_ctx__);
> + ub_ctx__ = NULL;
> + return;
> }
> #endif
Thanks for the improvement. It spurs a few thoughts.
It looks like part of this may be a somewhat independent bug fix
because, previously, ub_ctx__ was not deleted or set to NULL on error.
Is that part of this patch also a bug fix?
This looks a little unfair to Windows. I think that ub_ctx_resolvconf()
works on Windows if you supply a file name, or even if you pass NULL to
use the default Windows resolver parameters.
I think that the overall logic, then, could more fairly to Windows be
something like this:
const char *filename = getenv("OVS_RESOLV_CONF");
if (!filename) {
#ifdef _WIN32
/* On Windows, NULL means to use the system default nameserver. */
#else
filename = "/etc/resolv.conf";
#endif
}
The documentation for ub_ctx_resolvconf() is here:
https://linux.die.net/man/3/ub_ctx_resolvconf
(If we use this implementation, we need to be careful about error
messages, because NULL shouldn't be used for %s.)
The new environment variable should be documented in some appropriate
place.
Thanks a lot!
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
