Hi, I just wanted to ping on that one to make sure it does not get forgotten...
Best regards Christoph From: Langer, Christoph Sent: Donnerstag, 3. November 2016 16:46 To: 'net-dev@openjdk.java.net' <net-dev@openjdk.java.net> Subject: RE: RFR(M): 8167420: remove redundant code path in unix/native/libnet/Inet4AddressImpl.c 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?? 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? 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? Best regards Christoph From: Langer, Christoph Sent: Mittwoch, 2. November 2016 15:25 To: 'net-dev@openjdk.java.net' <net-dev@openjdk.java.net<mailto: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
