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