RFR(M): 8167420: remove redundant code path in unix/native/libnet/Inet4AddressImpl.c

Fri Nov 25 10:41:43 UTC 2016

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.hegarty at oracle.com]
>> Sent: Montag, 21. November 2016 15:18
>> To: Langer, Christoph <christoph.langer at sap.com>; net-dev at 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 at openjdk.java.net' <net-dev at 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 at openjdk.java.net' <net-dev at openjdk.java.net
>>> <mailto:net-dev at 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
>>>
>>>
>>>