RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()

[Ivan Gerasimov]

Hi Volker!

I'm not a Reviewer, but have a couple of minor comments.

In the C source files you changed the indentation to two spaces.
It looks inconsistent with other JDK sources.
I know that in hotspot they use two space indentation, but it's a 
different set of sources.


Inet4AddressImpl.c:
110   jboolean reverseLookup = (*env)->GetStaticBooleanField(env, 
ia_class, ia_doIPv4ReverseLookup);

Since doIPv4ReverseLookup never changes, wouldn't it make sense to 
declare jboolean reverseLookup static?
This way it would be retrieved only once.

The same in Inet6AddressImpl.c:
66   jboolean reverseLookup = (*env)->GetStaticBooleanField(env, 
ia_class, ia_doIPv6ReverseLookup);

And a static value retrieved here:
68   if ((*env)->GetStaticBooleanField(env, ia_class, 
ia_preferIPv6AddressID)) {

Sincerely yours,
Ivan

On 24.10.2014 18:47, Volker Simonis wrote:
> Hi,
>
> could somebody please have a quick look at this change.
> It's really not that complicated as it looks like from the comments -
> I just didn't manage to write it up in a more concise way :)
>
> Thanks,
> Volker
>
>
> On Thu, Oct 16, 2014 at 4:39 PM, Volker Simonis
> <volker.simonis at gmail.com> wrote:
>> Hi,
>>
>> could you please hava a look at the following change:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/8060470.v1
>> https://bugs.openjdk.java.net/browse/JDK-8060470
>>
>> It's probably easier to read the following in the webrev, but I copy
>> it below for discussion.
>>
>> Regards,
>> Volker
>>
>> So here comes the first version of this change. Its main focus is the
>> unification and simplification of
>> Inet{4,6}AddressImpl_getLocalHostName() (notice that these native
>> methods are currently only called from InetAddress.getLocalHost() so
>> the impact is manageable):
>>
>>   - Simplification: the current implementation has three versions of
>> this function: two versions of Inet4AddressImpl_getLocalHostName()
>> (one for "_ALLBSD_SOURCE && !HAS_GLIBC_GETHOSTBY_R" and another one
>> for the other Unix versions) and one version of
>> Inet6AddressImpl_getLocalHostName(). All these functions are very
>> similar and can be easily factored out into one new method.
>>   - Unification: there are subtle and probably unnecessary differences
>> between the IPv4 and IPv6 version of these methods which can be easily
>> eliminated.
>>
>> The only difference between the two IPv4 versions was the ai_family
>> flag passed as hint to the getaddrinfo() call. The Mac version used
>> AF_UNSPEC while the general Unix version used AF_INET. I don't see a
>> reason (and my tests didn't show any problems) why we couldn't use
>> AF_INET on MacOS as well.
>>
>> The IPv6 version used AF_UNSPEC as well. The new refactored method
>> getLocalHostName() which is now called from both, the single instance
>> of Inet4AddressImpl_getLocalHostName() and
>> Inet6AddressImpl_getLocalHostName() uses AF_INET in the IPv4 case
>> (which shouldn't change anything) and AF_INET6 for the IPv6 case.
>> Additionally, it uses the flag AI_V4MAPPED in the IPv6 case. This will
>> return an IPv4-mapped IPv6 addresses if no matching IPv6 addresses
>> could be found.
>>
>> The last difference between the old IPv4 and IPv6 versions was the
>> fact that the IPv4 versions always did a reverse lookup for the host
>> name. That means that after querying the hostname with gethostname(),
>> they used a call to getaddrinfo() to get the IP address of the host
>> name and finally they called getnameinfo() on that IP address to get
>> the host name once again. The IPv6 version only did this reverse
>> lookup on Solaris.
>>
>> It is unclear why this reverse lookup was necessary at all. Especially
>> if we take into account that the resulting host name will be only used
>> in InetAddress.getLocalHost() where it will finally serve as input to
>> InetAddressImpl.lookupAllHostAddr() which will in turn do exactly the
>> same reverse lookup procedure (at least if the default name service is
>> used). Therefore the new implementation switches this reverse lookup
>> off by default unless the user requests it with the two system
>> properties java.net.doIPv4ReverseLookupInGetLocalHost and
>> java.net.doIPv6ReverseLookupInGetLocalHost for IPv4 and IPv6
>> respectively. Consequently, the new Unix version of
>> Java_java_net_Inet{4,6}AddressImpl_getLocalHostName is now equal to
>> its Windows counterpart which simply returns the result of
>> gethostname() as well.
>>
>> Notice that for these properties, the name "IPv4" and "IPv6" refer to
>> the actual InetAddressImpl version (i.e. either Inet4AddressImpl or
>> Inet6AddressImpl) that is used by InetAddress. On most modern hosts,
>> InetAddress will use an Inet6AddressImpl helper if the host is IPv6
>> "capable". That doesn't necessarily mean that
>> InetAddress.getLocalHost() will return an IPv6 address or even that
>> there's a network interface with an IPv6 address - it just means that
>> the network stack can handle IPv6. InetAddress can be forced to use
>> the Inet4AddressImpl helper by setting the java.net.preferIPv4Stack
>> property to false.
>>
>> In the new implementation both properties
>> java.net.doIPv4ReverseLookupInGetLocalHost and
>> java.net.doIPv6ReverseLookupInGetLocalHost are set to false by
>> default. But the old behavior could be restored by setting
>> java.net.doIPv4ReverseLookupInGetLocalHost=true and
>> java.net.doIPv6ReverseLookupInGetLocalHost=true (Solaris only).
>>
>> In a previous mail thread [2] we discussed the possibility of
>> replacing the call to getnameinfo() in
>> Inet{4,6}AddressImpl_getLocalHostName() by simply using the
>> ai_canonname field returned by the getaddrinfo() call. But because I
>> think the reverse lookup is unnecessary anyway (and disabled by
>> default in the new implementation), I didn't tried that any more and
>> stayed with the old version.
>>
>> I've built these changes on Linux, Solaris, MacOS X and AIX and
>> manually tested them on various hosts with different IPv4/IPv6 setups
>> without any problems. In cases where the output of
>> InetAddress.getLocalHost() differed in the new version this was only a
>> difference in the hostname part of the InetAddress (i.e. fully
>> qualified name vs. simple name) and this could be easily restored with
>> the help of the new system properties. Notice that for the new version
>> the host name now usually corresponds to what the hostname command
>> returns on Unix which is probably what most people would expect. In
>> the old version the host name was more dependent on the local system
>> configuration (i.e. /etc/hosts or /etc/nsswitch.conf) as discussed in
>> [2].
>>
>> The following mail threads already discuss this issue:
>>
>> [1] http://mail.openjdk.java.net/pipermail/net-dev/2014-October/thread.html#8721
>> [2] http://mail.openjdk.java.net/pipermail/net-dev/2013-June/thread.html#6543
>>
>> PS: We probably shouldn't discuss this too long as there are other
>> methods like Java_java_net_Inet{4,6}AddressImpl_lookupAllHostAddr
>> which are waiting for unification and simplification as well :)
>