Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Aleksei, Daniel, Great news, thanks a lot for all your help on cleaning up this patch! Cheers, - Anuraag On Wed, Jan 15, 2020, 01:14 Aleks Efimov wrote: > Thanks Daniel! > > Anuuraag, > I will update the copyrights, no need to send new patch. Then will > sponsor your change - commit and pu

Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Thanks Daniel! Anuuraag, I will update the copyrights, no need to send new patch.  Then will sponsor your change - commit and push to jdk/jdk. Best Regards, Aleksei On 14/01/2020 11:06, Daniel Fuchs wrote: Hi Anuuraag, Aleksei, Looks good to me. Except that now copyright years must be up

Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuuraag, Aleksei, Looks good to me. Except that now copyright years must be updated to 2020. No need for a new webrev if that's the only change! best regards, -- daniel On 09/01/2020 17:17, Aleks Efimov wrote: Hi Anuuraag, Latest webrev: http://cr.openjdk.java.net/~aefimov/anuraaga/7006

e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuuraag, Latest webrev: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/05/ Looks fine to me CI is also happy Best Regards, Aleksei On 09/01/2020 15:22, Anuraag Agrawal wrote: Hi all, Apologies for the spam. I made one more tweak to follow up the previous, and removed the default va

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi all, Apologies for the spam. I made one more tweak to follow up the previous, and removed the default value of -1 for lastRefresh. Now that there is no semantic meaning to -1, I think unspecified default (effectively zero) is more appropriate and easy to reason about. Inline patch follows dif

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Daniel, Thanks for the review! On Thu, Jan 9, 2020 at 9:04 PM Daniel Fuchs wrote: > Hi, > > Two small remarks: > > ResolverConfigurationImpl.java: > >71 if (!l.contains(s)) { > > Maybe empty strings should be skipped here too. > Makes sense, added the check. > > I believe

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi, Two small remarks: ResolverConfigurationImpl.java: 71 if (!l.contains(s)) { Maybe empty strings should be skipped here too. I believe it would be more correct to change this line: 117 if (lastRefresh >= 0) { to 117 if (lastRefresh != -1) { as Sy

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

I’m happy with the latest iteration of this code. It has my Review. -Chris. > On 9 Jan 2020, at 00:54, Aleks Efimov wrote: > > Got the testing results: the CI is happy with the last patch - no JNDI test > failures observed > > Kind Regards, > Aleksei > > On 08/01/2020 18:23, Aleks Efimov wro

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Got the testing results: the CI is happy with the last patch - no JNDI test failures observed Kind Regards, Aleksei On 08/01/2020 18:23, Aleks Efimov wrote: Hi Anuraag, I've uploaded your latest patch to the following location: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/04 The loca

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuraag, I've uploaded your latest patch to the following location: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/04 The local Windows build and the acquired configuration are good. I will run you patch through our CI system and will update this thread once I get the results. Kind

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Chris, Ah ok - I was worried it might be a backwards incompatible change, but I guess these are internal classes anyways. I have updated the patch, removing the exception handling logic so the errors propagate up. Inline patch follows diff --git a/src/java.base/windows/classes/sun/net/dns/Res

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

> On 8 Jan 2020, at 05:12, Anuraag Agrawal wrote: > > Hi Chris, > > Happy new year - sorry for the long delay. I have made an updated patch > fixing the comment (missing comma), bumping MAX_STR_LEN, and tweaking the > exception behavior. > > Basically, as you pointed out loadDNSConfig itself

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Chris, Happy new year - sorry for the long delay. I have made an updated patch fixing the comment (missing comma), bumping MAX_STR_LEN, and tweaking the exception behavior. Basically, as you pointed out loadDNSConfig itself does not throw any exception. The getAdapters function will throw a) O

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

> On 27 Dec 2019, at 08:44, Anuraag Agrawal wrote: > > ... > On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty > wrote: > > ... > I think that this mainly looks good. A few small comments: > > 1) If getAdapters returns an error ( ret != ERROR_SUCCESS ), then the

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Chris, Thanks for the comments! On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty wrote: > > > > On 24 Dec 2019, at 10:12, Aleks Efimov > wrote: > > > > Hi Anuraag, > > > > We need additional approval from openjdk reviewer. After that I will > sponsor your change. > > > > Merry Christmas and a

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

> On 24 Dec 2019, at 10:12, Aleks Efimov wrote: > > Hi Anuraag, > > We need additional approval from openjdk reviewer. After that I will sponsor > your change. > > Merry Christmas and a Happy New Year, > Aleksei > > On 18/12/2019 06:08, Anuraag Agrawal wrote: >> ... >> http://cr.openjdk.ja

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Aleksei, Thanks for the feedback. I had thought about it but felt that it might be better to leave as much business logic to Java vs native as possible as a general principle. But if it makes more sense to do it native here happy to make the change :) Thanks, - Anuraag On Thu, Dec 12, 2019, 0

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuraag, The webrev with the latest patch can be viewed here: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/02 Thanks for replacing FirstDnsSuffix usages with DnsSuffix. I've double checked it with native application and can confirm that on my test host (Microsoft Windows Server 2016

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Aleksei, Thanks for running the test. I had checked the global search list, but realize I didn't check connection-specific suffixes which were causing the issue you see. While the documentation states that FirstDnsSuffix is populated on Vista+, amazingly it doesn't seem to be https://docs.micr

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuraag, The webrev with your latest changes can be found here:     http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/01 The copyright changes looks good. The split by space also looks good with assumption that we will never get string with two spaces between IP addresses/DNS suffixes.

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi all, Thanks for the reviews. I've fixed the formatting errors, and I hope I fixed the copyright issue. I couldn't find documentation on the format of the copyright header, but assume the second number is the year of modification to update. I found ResolverConfigurationImpl.c to have an old one

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

On 06/12/2019 15:34, Aleks Efimov wrote: : I've created webrev with your latest changes - it could be convenient for other reviewers: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/00 I will happily sponsor this change once it gets necessary approvals This could be split into two issues

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hey Anuraag, I've looked at the failures and I believe that your second patch accidentally removed 'loadDNSconfig0()' native call. Putting it back to src/java.base/windows/classes/sun/net/dns/ResolverConfigurationImpl.java:145 resolves all these failures. -Aleksei On 02/12/2019 13:04, Alek

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Anuraag, I've submited your patch to our CI system and I'm observing a bunch of NPE failures with such stack trace: java.lang.NullPointerException     at java.base/sun.net.dns.ResolverConfigurationImpl.allocateListForDelimitedString(ResolverConfigurationImpl.java:108)     at java.base/sun.

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hi Bernd, Thanks for the look and suggestion. I have updated those comments, as well as added several more to explain the native code aspects better which I hope helps future readers of the code. Hope it looks better now. Inline patch follows diff --git a/src/java.base/windows/classes/sun/net/dn

Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

Hello Anuraag, The patch looks like a good idea (especially the additional cleanup for nanos and arraylist) Just a minor thing I noticed there are two comments talking about „comma and space“ but the code below will only tokenize by space. Imguess the comment needs to be fixed, for the next rev