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. :) 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? 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-dev@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 > > > > > >
