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

[Chris Hegarty]

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
>
>
>