Hi, please finally review the change for 8167420. (Windows counterpart of 8167457)
I made a new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/ In the new version I also touched the isReachable coding to do some cleanups and fix coverity code scan issues. I also updated Bug 8167420 and enumerated all significant changes in its description. I did quite some extensive testing in our landscape and would ask you to run it through JPRT testing as well. @Chris: I did not touch the getLocalHostName behavior at all, this should probably be subject to a later change. So it'll do getnameinfo/getaddrinfo for all unix platforms for inet4 and only for solaris for inet6. Thanks a lot in advance Christoph > -----Original Message----- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Freitag, 25. November 2016 11:42 > To: Langer, Christoph <christoph.lan...@sap.com>; net-dev@openjdk.java.net > Subject: Re: RFR(M): 8167420: remove redundant code path in > unix/native/libnet/Inet4AddressImpl.c > > On 21/11/16 14:58, Langer, Christoph wrote: > > Hi Chris, > > > > thanks for looking into this. > > > > First of all I'm glad that you agree to make the getLocalHostname call > consistent throughout all implementations. :) > > Yes, consistency is good. > > > However, my feeling at the moment rather is to prefer just return the > gethostname() API result instead of doing getaddrinfo/getnameinfo on top. Is > there anything speaking against that? Maybe a Java property could be > introduced to enable getaddrinfo/getnameinfo in case some scenario would > need that? > > AFAIK getaddrinfo/getnameinfo was only ever added to support Solaris / > NIS. I will need to check the latest state of this before being able > to answer. If we remove getaddrinfo/getnameinfo, then maybe this is > something that could be done early in JDK 10, so that it gets a long > time to bake. ( The JDK 10 forest is due open very soon ). > > -Chris. > > > Best regards > > Christoph > > > > > > > >> -----Original Message----- > >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >> Sent: Montag, 21. November 2016 15:18 > >> To: Langer, Christoph <christoph.lan...@sap.com>; net- > d...@openjdk.java.net > >> Subject: Re: RFR(M): 8167420: remove redundant code path in > >> unix/native/libnet/Inet4AddressImpl.c > >> > >> Hi Christoph, > >> > >> On 03/11/16 15:46, Langer, Christoph wrote: > >>> Hi again, > >>> > >>> I have to make one addition to my points: > >>> > >>> Java_java_net_Inet6AddressImpl_getLocalHostName: > >>> > >>> - made getaddrinfo/getnameinfo turnaround the default, before > >>> it was only used on solaris. But it should be the default since > >>> Inet4Addressimpl does it as well?? Or do we want to remove it > completely?? > >> > >> I'd say make it the default, which is what you have in your webrev. > >> It seems mostly redundant on platforms that return the FQDN, but > >> being consistent would be better. > >> > >>> For this one I just had a customer case where I really had a hard time > >>> to figure out why on one of his Linux machines InetAddres.getLocaHost() > >>> would return an FQDN and on the other it wouldn't. It eventually turned > >>> out that on the machine where FQDN was returned, no IPv4 addresses > were > >>> configured and hence he used the Inet4AddressImpl version to do the > >>> getaddrinfo/getnameinfo to lookup the FQDN. On his other machine, > where > >>> IPv6 is configured, the Inet6AddressImpl variant is used and we only get > >>> the short name. So I really think this should be aligned, shouldn't it? > >> > >> Yes. > >> > >>> Maybe it's really better to always return what the gethostname API says. > >>> Or is there a reason why IPv4 and IPv6 handling is different? > >> > >> I cannot find any specific reason why these would be different. > >> > >> -Chris. > >> > >>> Best regards > >>> > >>> Christoph > >>> > >>> > >>> > >>> *From:*Langer, Christoph > >>> *Sent:* Mittwoch, 2. November 2016 15:25 > >>> *To:* 'net-dev@openjdk.java.net' <net-dev@openjdk.java.net> > >>> *Subject:* RFR(M): 8167420: remove redundant code path in > >>> unix/native/libnet/Inet4AddressImpl.c > >>> > >>> > >>> > >>> Hi, > >>> > >>> > >>> > >>> I respun my proposal for cleaning up Inet4AddressImpl.c and > >>> Inet6AddressImpl.c: > >>> > >>> http://cr.openjdk.java.net/~clanger/webrevs/8167420.1/ > >>> > >>> > >>> > >>> So, apart from dropping the implementation for the rare AllBSD/MacOS > >>> code, I suggest the following things: > >>> > >>> > >>> > >>> Java_java_net_Inet4AddressImpl_getLocalHostName: > >>> > >>> - rare mac version used AF_UNSPEC for getaddrinfo call, new > >>> version uses AF_INET which is probably more correct > >>> > >>> > >>> > >>> Java_java_net_Inet4AddressImpl_lookupAllHostAddr: > >>> > >>> - remove isspace(hostname[0]) check (solaris and strange mac version had > >>> it). It should check if the hostname starts with a blank and throw an > >>> UnknownHostException in that case. However, my testing on current > >>> Solaris and MacOS versions shows me that it is not needed and the > >>> UnknownHostException is thrown anyway. > >>> > >>> - standard version now has the MacOS fallback "lookupIfLocalhost" (which > >>> only strange mac version had before). This function lookupifLocalhost is > >>> called if getnameinfo returns with an error and localhost addresses > >>> shall be determined. Then getaddrinfo would be used to enumerate local > >>> addresses. However, I would generally question that call - or we could > >>> as well have an implementation of that fallback based on the > >>> NetworkInterface class. Any oppinions? > >>> > >>> > >>> > >>> Java_java_net_Inet6AddressImpl_getLocalHostName: > >>> > >>> - made getaddrinfo/getnameinfo turnaround the default, before > >>> it was only used on solaris. But it should be the default since > >>> Inet4Addressimpl does it as well?? Or do we want to remove it > completely?? > >>> > >>> > >>> > >>> Java_java_net_Inet6AddressImpl_lookupAllHostAddr: > >>> > >>> - remove isspace check (for solaris), same reasons as with > >>> Java_java_net_Inet4AddressImpl_lookupAllHostAddr > >>> > >>> > >>> > >>> Java_java_net_Inet6AddressImpl_getHostByAddr: > >>> > >>> - replace CHECK_NULL_RETURN(ret, NULL) with > >>> UnknownHostException in case of getnameinfo error. This seems more > >>> correct if you look at the usage of getHostByAddr in InetAddress.java > >>> > >>> > >>> > >>> My local tests did not show any problems. I'll add the fix to the patch > >>> queues in our internal OpenJDK build environment to see if problems > >>> appear. I would appreciate any feedback on the points I elaborated above. > >>> > >>> > >>> > >>> Thanks & Best regards > >>> > >>> Christoph > >>> > >>> > >>> > >>> > >>> > >>> *From:*Langer, Christoph > >>> *Sent:* Montag, 10. Oktober 2016 09:32 > >>> *To:* 'net-dev@openjdk.java.net' <net-dev@openjdk.java.net > >>> <mailto:net-dev@openjdk.java.net>> > >>> *Subject:* RFR(S): 8167420: remove redundant code path in > >>> unix/native/libnet/Inet4AddressImpl.c > >>> > >>> > >>> > >>> Hi, > >>> > >>> > >>> > >>> here's another review request for a cleanup: > >>> > >>> > >>> > >>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167420.0/ > >>> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8167420 > >>> > >>> > >>> > >>> In unix/native/libnet/Inet4AddressImpl.c, there exist 2 implementations > >>> for each of Java_java_net_Inet4AddressImpl_getLocalHostName, > >>> Java_java_net_Inet4AddressImpl_lookupAllHostAddr and > >>> Java_java_net_Inet4AddressImpl_getHostByAddr. I think one branch is > >>> obsolete. > >>> > >>> > >>> > >>> I also did some cleanups in those functions. One question that I still > >>> have is if we should add the MACOSX workaround path when getaddrinfo > >>> returns error and "lookupIfLocalhost" is called? However, as it was not > >>> part of the standard branch which is probably used mostly on MacOSX > >>> nowadays, I tend to remove it. In the webrev it is still contained. > >>> > >>> > >>> > >>> The changeset is based on my proposal for 8167295 > >>> <https://bugs.openjdk.java.net/browse/JDK-8167295> > >>> (http://cr.openjdk.java.net/~clanger/webrevs/8167295.1/). > >>> > >>> > >>> > >>> Thanks & Best regards > >>> > >>> Christoph > >>> > >>> > >>>
