Re: Patch for fixing the slow DNS lookup issue
The patched code have been running in our production environment for half year. I consider it is ok personally. This patch didn't be merged due to lack of test case. I hope some can help to finish this job. On Thu, Nov 6, 2014 at 5:26 PM, Jakub Hrozek wrote: > On Tue, Nov 04, 2014 at 01:27:38PM -0500, Brad House wrote: > > On 5/22/14, 1:43 AM, Lei Shi wrote: > > >Hello, everyone > > > > > >This patch include two major change groups. one is fixing the dns > lookup issue due to dummy dns information of a > > >disconnected adapter(in my case is a bluetooth adapter). I changed the > dns lookup policy to try GetNetworkParams first > > >because the GetNetworkParams provides the most reliable dns > information(lots of checks were done by system). > > >I also filter out inoperable adapter in DNS_AdaptersAddresses in case > GetNetworkParams fail. > > >the other is explicit invoke ANSI version Win32 API in case compile > c-ares in unicode environment. > > > > > >Best Wishes > > >Lei Shi. > > > > > > > I just had a report of a similar issue from a customer complaining that > DNS lookups were slow across multiple > > machines running c-ares 1.10.0, but not from machines running much older > versions of c-ares 1.5.3. I haven't > > fully investigated since I don't have access to their machines, but it > is very likely in their environment that > > they could have some disabled interfaces with bogus server addresses > which this patch appears to address. I > > know c-ares completely changed the way windows DNS servers are looked up > between those versions. > > > > I checked the Git repo and it doesn't appear a patch similar to this > ever made it upstream. Did this get > > dropped? Has anyone else tested this patch and found it to be improper, > or if there was a better way > > to handle it? > > (Speaking only for myself now..) > > The patch touches Windows code which is something I have personally no > means of testing. I can help with general code review, but I'm not able > to test the patch. >
Re: Patch for fixing the slow DNS lookup issue
On Tue, Nov 04, 2014 at 01:27:38PM -0500, Brad House wrote: > On 5/22/14, 1:43 AM, Lei Shi wrote: > >Hello, everyone > > > >This patch include two major change groups. one is fixing the dns lookup > >issue due to dummy dns information of a > >disconnected adapter(in my case is a bluetooth adapter). I changed the dns > >lookup policy to try GetNetworkParams first > >because the GetNetworkParams provides the most reliable dns information(lots > >of checks were done by system). > >I also filter out inoperable adapter in DNS_AdaptersAddresses in case > >GetNetworkParams fail. > >the other is explicit invoke ANSI version Win32 API in case compile c-ares > >in unicode environment. > > > >Best Wishes > >Lei Shi. > > > > I just had a report of a similar issue from a customer complaining that DNS > lookups were slow across multiple > machines running c-ares 1.10.0, but not from machines running much older > versions of c-ares 1.5.3. I haven't > fully investigated since I don't have access to their machines, but it is > very likely in their environment that > they could have some disabled interfaces with bogus server addresses which > this patch appears to address. I > know c-ares completely changed the way windows DNS servers are looked up > between those versions. > > I checked the Git repo and it doesn't appear a patch similar to this ever > made it upstream. Did this get > dropped? Has anyone else tested this patch and found it to be improper, or > if there was a better way > to handle it? (Speaking only for myself now..) The patch touches Windows code which is something I have personally no means of testing. I can help with general code review, but I'm not able to test the patch.
Re: Patch for fixing the slow DNS lookup issue
On 5/22/14, 1:43 AM, Lei Shi wrote: Hello, everyone This patch include two major change groups. one is fixing the dns lookup issue due to dummy dns information of a disconnected adapter(in my case is a bluetooth adapter). I changed the dns lookup policy to try GetNetworkParams first because the GetNetworkParams provides the most reliable dns information(lots of checks were done by system). I also filter out inoperable adapter in DNS_AdaptersAddresses in case GetNetworkParams fail. the other is explicit invoke ANSI version Win32 API in case compile c-ares in unicode environment. Best Wishes Lei Shi. I just had a report of a similar issue from a customer complaining that DNS lookups were slow across multiple machines running c-ares 1.10.0, but not from machines running much older versions of c-ares 1.5.3. I haven't fully investigated since I don't have access to their machines, but it is very likely in their environment that they could have some disabled interfaces with bogus server addresses which this patch appears to address. I know c-ares completely changed the way windows DNS servers are looked up between those versions. I checked the Git repo and it doesn't appear a patch similar to this ever made it upstream. Did this get dropped? Has anyone else tested this patch and found it to be improper, or if there was a better way to handle it? Thanks. -Brad
Re: Patch for fixing the slow DNS lookup issue
On Fri, Jul 25, 2014 at 12:03:27PM +0200, Nikos Mavrogiannopoulos wrote: > On Fri, 2014-07-25 at 11:13 +0200, Jakub Hrozek wrote: > > > > https://github.com/bagder/c-ares/pulls > > > > https://github.com/bagder/c-ares/pull/16 - I will ask my RH colleagues > > about this. There is an effort around DNSSEC in Red Hat development now, > > but I admit my DNSSEC knowledge is very limited, so I don't feel > > qualified for a review. As a general note, this should be discussed with > > the libc folks at the libc-alpha list. > > The co-ordination with the glibc folks would be nice to occur in order > to have a consistent way to read the trusted nameservers for dnssec. > These servers need to be marked separately in order to allow the system > administrator to trust the local verifying unbound server, and not the > dns server of the hotel he just got DHCP, for dnssec verification. This > is important as the patch adds non-validating dnssec support and relies > on the upstream server to do validation; the advantage is that it avoids > any crypto dependencies. > > Unfortunately the (months-long) discussion on libc-alpha didn't end in > anything productive, hence I implemented what I thought best, i.e., a > separate resolv-sec.conf file. That part is separated from the rest of > the functionality (the last patch in pull request), and I'd be happy to > update it if you have a better idea. > > If you have better communication skills than me you may want to resume > the discussion in libc-alpha (or some other libc people like the > freebsd). I will first try to talk to Petr Spacek, who is the DNS guy on our team before talking to the glibc people.. > Nevertheless, in glibc my understanding is that they don't > plan to implement anything dnssec related anytime soon, so even if an > agreement is made that may not binding to them. Overall, I think it > would be nice for c-ares to have that functionality even if glibc > doesn't. Right, last time I heard, even systemd folks were dabbling with the idea. I personally don't have a problem with out-of-glibc implementation, after all, c-ares is a parallel DNS stack as well. What I would like to avoid is a scenario where you would configure DNSSEC by following steps A,B,C for c-ares and steps X,Y,Z for systemd/glibc/whatever.
Re: Patch for fixing the slow DNS lookup issue
On Fri, 2014-07-25 at 11:13 +0200, Jakub Hrozek wrote: > > https://github.com/bagder/c-ares/pulls > > https://github.com/bagder/c-ares/pull/16 - I will ask my RH colleagues > about this. There is an effort around DNSSEC in Red Hat development now, > but I admit my DNSSEC knowledge is very limited, so I don't feel > qualified for a review. As a general note, this should be discussed with > the libc folks at the libc-alpha list. The co-ordination with the glibc folks would be nice to occur in order to have a consistent way to read the trusted nameservers for dnssec. These servers need to be marked separately in order to allow the system administrator to trust the local verifying unbound server, and not the dns server of the hotel he just got DHCP, for dnssec verification. This is important as the patch adds non-validating dnssec support and relies on the upstream server to do validation; the advantage is that it avoids any crypto dependencies. Unfortunately the (months-long) discussion on libc-alpha didn't end in anything productive, hence I implemented what I thought best, i.e., a separate resolv-sec.conf file. That part is separated from the rest of the functionality (the last patch in pull request), and I'd be happy to update it if you have a better idea. If you have better communication skills than me you may want to resume the discussion in libc-alpha (or some other libc people like the freebsd). Nevertheless, in glibc my understanding is that they don't plan to implement anything dnssec related anytime soon, so even if an agreement is made that may not binding to them. Overall, I think it would be nice for c-ares to have that functionality even if glibc doesn't. regards, Nikos
Re: Patch for fixing the slow DNS lookup issue
On Fri, Jul 25, 2014 at 10:50:47AM +0200, Daniel Stenberg wrote: > On Fri, 25 Jul 2014, David Drysdale wrote: > > >Pushed. Hopefully it might help a bit. > > Speaking of that, we have three old pull requests pending: > > https://github.com/bagder/c-ares/pulls https://github.com/bagder/c-ares/pull/16 - I will ask my RH colleagues about this. There is an effort around DNSSEC in Red Hat development now, but I admit my DNSSEC knowledge is very limited, so I don't feel qualified for a review. As a general note, this should be discussed with the libc folks at the libc-alpha list. https://github.com/bagder/c-ares/pull/17 - This one requires Windows.. https://github.com/bagder/c-ares/pull/18 - I don't like this one tbh, there are platforms that don't have socklen_t (old Solaris, old HP-UX). Because I don't have access to either of these, I can't test the patch, but the text at the bottom of "man 2 accept" seems to suggest we might break these old platforms..
Re: Patch for fixing the slow DNS lookup issue
On Fri, 25 Jul 2014, David Drysdale wrote: Pushed. Hopefully it might help a bit. Speaking of that, we have three old pull requests pending: https://github.com/bagder/c-ares/pulls -- / daniel.haxx.se
Re: Patch for fixing the slow DNS lookup issue
On Fri, Jul 25, 2014 at 09:34:23AM +0100, David Drysdale wrote: > How about the attached? > > From ede0f84b8e9cfe4eeaafb1c90e5fea006e19fe5e Mon Sep 17 00:00:00 2001 > From: David Drysdale > Date: Fri, 25 Jul 2014 09:28:46 +0100 > Subject: [PATCH] CONTRIBUTING: add file to indicate mailing list is preferred Looks good to me! (Although I'm not a native speaker) > > --- > CONTRIBUTING | 11 +++ > 1 file changed, 11 insertions(+) > create mode 100644 CONTRIBUTING > > diff --git a/CONTRIBUTING b/CONTRIBUTING > new file mode 100644 > index ..c7dda05db014 > --- /dev/null > +++ b/CONTRIBUTING > @@ -0,0 +1,11 @@ > +Contributing to c-ares > +== > + > +The c-ares developers prefer patches to be sent to the c-ares mailing list > +rather than receiving pull requests via GitHub. So for suggested changes > +please: > + > + - Subscribe to the mailing list at: > + http://cool.haxx.se/mailman/listinfo/c-ares > + - Use 'git format-patch' to generate patch files. > + - Send the patches to the mailing list at c-ares@cool.haxx.se > -- > 2.0.0.526.g5318336 >
Re: Patch for fixing the slow DNS lookup issue
On Thu, Jul 24, 2014 at 05:11:41PM +0100, David Drysdale wrote: > On Mon, May 26, 2014 at 9:08 AM, Jakub Hrozek wrote: > > > On Sat, May 24, 2014 at 02:29:16PM +0200, Daniel Stenberg wrote: > > > On Thu, 22 May 2014, Jakub Hrozek wrote: > > > > > > >I can't comment on the patch contents myself at all, because I'm > > > >not a Windows developer, but I guess it would be easier for others > > > >to review if the patch was a git-formatted one or a github pull > > > >request since c-ares uses git and github anyway. > > > > > > I generally discourage github pull requests simply because it then > > > only alerts those who subscribe to those on github (and it isn't > > > easy for us to tell who got it or care about it) and it doesn't send > > > the patch here for review. > > > > Yeah, this is my pain point about github as well..if only there was a > > way to securely enable sending all pull requests to a mailing list..but > > unfortunately I couldn't find one that would also make it hard to > > request password changes to be sent to the list. > > > As a slight mitigation, is it worth adding a CONTRIBUTING file at the top > level advising people to send patches to the mailing list? > > According to the GitHub help, adding that file should at least set up a > notice that potential contributors might spot: > https://help.github.com/articles/how-do-i-set-up-guidelines-for-contributors Good idea, there already is a note about this list in the README that is displayed when you navigate to https://github.com/bagder/c-ares but making this info more prominent certainly wouldn't hurt.
Re: Patch for fixing the slow DNS lookup issue
On Sat, May 24, 2014 at 02:29:16PM +0200, Daniel Stenberg wrote: > On Thu, 22 May 2014, Jakub Hrozek wrote: > > >I can't comment on the patch contents myself at all, because I'm > >not a Windows developer, but I guess it would be easier for others > >to review if the patch was a git-formatted one or a github pull > >request since c-ares uses git and github anyway. > > I generally discourage github pull requests simply because it then > only alerts those who subscribe to those on github (and it isn't > easy for us to tell who got it or care about it) and it doesn't send > the patch here for review. Yeah, this is my pain point about github as well..if only there was a way to securely enable sending all pull requests to a mailing list..but unfortunately I couldn't find one that would also make it hard to request password changes to be sent to the list. > > It is a bit tricky how to handle these sorts of patches that don't > get any review/use from more than the submitter. I have no good > solution for that, but I've applied such patches in the past only > based on how they look. I agree, maybe it would help if a unit test was also submitted?
Re: Patch for fixing the slow DNS lookup issue
On Thu, 22 May 2014, Jakub Hrozek wrote: I can't comment on the patch contents myself at all, because I'm not a Windows developer, but I guess it would be easier for others to review if the patch was a git-formatted one or a github pull request since c-ares uses git and github anyway. I generally discourage github pull requests simply because it then only alerts those who subscribe to those on github (and it isn't easy for us to tell who got it or care about it) and it doesn't send the patch here for review. It is a bit tricky how to handle these sorts of patches that don't get any review/use from more than the submitter. I have no good solution for that, but I've applied such patches in the past only based on how they look. -- / daniel.haxx.se
Re: Patch for fixing the slow DNS lookup issue
On Thu, May 22, 2014 at 01:43:53PM +0800, Lei Shi wrote: > Hello, everyone > > This patch include two major change groups. one is fixing the dns lookup > issue due to dummy dns information of a disconnected adapter(in my case is > a bluetooth adapter). I changed the dns lookup policy to try > GetNetworkParams first because the GetNetworkParams provides the most > reliable dns information(lots of checks were done by system). > I also filter out inoperable adapter in DNS_AdaptersAddresses in case > GetNetworkParams fail. > the other is explicit invoke ANSI version Win32 API in case compile c-ares > in unicode environment. > > Best Wishes > Lei Shi. Hi, I can't comment on the patch contents myself at all, because I'm not a Windows developer, but I guess it would be easier for others to review if the patch was a git-formatted one or a github pull request since c-ares uses git and github anyway.
Patch for fixing the slow DNS lookup issue
Hello, everyone This patch include two major change groups. one is fixing the dns lookup issue due to dummy dns information of a disconnected adapter(in my case is a bluetooth adapter). I changed the dns lookup policy to try GetNetworkParams first because the GetNetworkParams provides the most reliable dns information(lots of checks were done by system). I also filter out inoperable adapter in DNS_AdaptersAddresses in case GetNetworkParams fail. the other is explicit invoke ANSI version Win32 API in case compile c-ares in unicode environment. Best Wishes Lei Shi. dns.patch Description: Binary data
