On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff <b...@ovn.org> wrote:
> [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?
>
It is a fix based on your previous suggestion: DNS resolving should stop
if there is no DNS server configured. Do you think we should create another
patch only for this 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.)
>
Good point, will do.
>
> The new environment variable should be documented in some appropriate
> place.
>
Sure, will do. Thanks.
>
> Thanks a lot!
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
