Re: [ovs-dev] [PATCH] dns-resolve: Stop dns resolving when dns servers are configured.
Sure, will do. Thanks for the comments.
Best,
Yifeng
On Tue, Nov 6, 2018 at 10:04 AM Ben Pfaff 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 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
> > > > Suggested-by: Mark Michelson
> > > > Signed-off-by: Yifeng Sun
> > > > ---
> > > > 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, ) || errno != ENOENT) {
> > > > int retval = ub_ctx_resolvconf(ub_ctx__, filename);
> > > > if (retval != 0) {
> > > > VLOG_WARN_RL(, "Failed to read %s: %s",
> > > > filename, ub_strerror(retval));
> > > > +ub_ctx_delete(ub_ctx__);
> > > > +ub_ctx__ = NULL;
> > > > +return;
> > > > }
> > > > +} else {
> > > > +VLOG_WARN_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.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dns-resolve: Stop dns resolving when dns servers are configured.
On Tue, Nov 06, 2018 at 09:48:36AM -0800, Yifeng Sun wrote:
> On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff 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
> > > Suggested-by: Mark Michelson
> > > Signed-off-by: Yifeng Sun
> > > ---
> > > 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, ) || errno != ENOENT) {
> > > int retval = ub_ctx_resolvconf(ub_ctx__, filename);
> > > if (retval != 0) {
> > > VLOG_WARN_RL(, "Failed to read %s: %s",
> > > filename, ub_strerror(retval));
> > > +ub_ctx_delete(ub_ctx__);
> > > +ub_ctx__ = NULL;
> > > +return;
> > > }
> > > +} else {
> > > +VLOG_WARN_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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dns-resolve: Stop dns resolving when dns servers are configured.
On Mon, Nov 5, 2018 at 7:58 PM Ben Pfaff 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
> > Suggested-by: Mark Michelson
> > Signed-off-by: Yifeng Sun
> > ---
> > 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, ) || errno != ENOENT) {
> > int retval = ub_ctx_resolvconf(ub_ctx__, filename);
> > if (retval != 0) {
> > VLOG_WARN_RL(, "Failed to read %s: %s",
> > filename, ub_strerror(retval));
> > +ub_ctx_delete(ub_ctx__);
> > +ub_ctx__ = NULL;
> > +return;
> > }
> > +} else {
> > +VLOG_WARN_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
Re: [ovs-dev] [PATCH] dns-resolve: Stop dns resolving when dns servers are configured.
[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
> Suggested-by: Mark Michelson
> Signed-off-by: Yifeng Sun
> ---
> 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, ) || errno != ENOENT) {
> int retval = ub_ctx_resolvconf(ub_ctx__, filename);
> if (retval != 0) {
> VLOG_WARN_RL(, "Failed to read %s: %s",
> filename, ub_strerror(retval));
> +ub_ctx_delete(ub_ctx__);
> +ub_ctx__ = NULL;
> +return;
> }
> +} else {
> +VLOG_WARN_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
[ovs-dev] [PATCH] dns-resolve: Stop dns resolving when dns servers are configured.
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
Suggested-by: Mark Michelson
Signed-off-by: Yifeng Sun
---
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, ) || errno != ENOENT) {
int retval = ub_ctx_resolvconf(ub_ctx__, filename);
if (retval != 0) {
VLOG_WARN_RL(, "Failed to read %s: %s",
filename, ub_strerror(retval));
+ub_ctx_delete(ub_ctx__);
+ub_ctx__ = NULL;
+return;
}
+} else {
+VLOG_WARN_RL(, "Failed to read %s: %s",
+ filename, ovs_strerror(errno));
+ub_ctx_delete(ub_ctx__);
+ub_ctx__ = NULL;
+return;
}
#endif
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
