| Message ID | 1541463618-3832-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dns-resolve: Stop dns resolving when dns servers are configured. | expand |
[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 <blp@ovn.org> > Suggested-by: Mark Michelson <mmichels@redhat.com> > Signed-off-by: Yifeng Sun <pkusunyifeng@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!
On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff <blp@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 <blp@ovn.org> > > Suggested-by: Mark Michelson <mmichels@redhat.com> > > Signed-off-by: Yifeng Sun <pkusunyifeng@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! >
On Tue, Nov 06, 2018 at 09:48:36AM -0800, Yifeng Sun wrote: > On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff <blp@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 <blp@ovn.org> > > > Suggested-by: Mark Michelson <mmichels@redhat.com> > > > Signed-off-by: Yifeng Sun <pkusunyifeng@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? Oh, I did not realize that this was the change that implemented that. If it is not troublesome, then I would divide this into two patches, because that would make it clear which part of the patch implements each change. Thanks for the other responses too. Thanks, Ben.
Sure, will do. Thanks for the comments. Best, Yifeng On Tue, Nov 6, 2018 at 10:04 AM Ben Pfaff <blp@ovn.org> wrote: > On Tue, Nov 06, 2018 at 09:48:36AM -0800, Yifeng Sun wrote: > > On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff <blp@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 <blp@ovn.org> > > > > Suggested-by: Mark Michelson <mmichels@redhat.com> > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@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? > > Oh, I did not realize that this was the change that implemented that. > > If it is not troublesome, then I would divide this into two patches, > because that would make it clear which part of the patch implements each > change. > > Thanks for the other responses too. > > Thanks, > > Ben. >
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
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 <blp@ovn.org> Suggested-by: Mark Michelson <mmichels@redhat.com> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- lib/dns-resolve.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)