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

[Ivan Gerasimov]

On 27.10.2014 21:45, Volker Simonis wrote:
> On Fri, Oct 24, 2014 at 7:07 PM, Ivan Gerasimov
> <ivan.gerasimov at oracle.com> wrote:
>> 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.
>>
> Well, the problem is that already that very file contains code in both
> code conventions (see for example the implementations of 'ping4()'
> and 'Java_java_net_Inet4AddressImpl_isReachable0()' in
> Inet4AddressImpl.c which are mostly indented by two spaces). I have no
> problems to adhere to any convention as long as it is generally
> obeyed. As this does not seemed to be the case in these files, I've
> just chosen what I thought is most appropriate. So to keep a long
> story short - I can either:
>
>   1. indent my changes to four spaces (which will still let the files
> with mixed indentation)
>   2. change all indentation in the file to two spaces
>   3. change all indentation in the file to four spaces
>
> Please just tell me what you'd prefer.

I think #1  it the most appropriate here.

>> 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)) {
>>
> I think simply declaring the mentioned variables static is not
> possible in "C" and this is a "C" file compiled with a "C" compiler.
> You would get a "initializer element is not constant" error (see for
> example http://stackoverflow.com/questions/5921920/difference-between-initialization-of-static-variables-in-c-and-c).
> I could of course use a second static varibale to do the
> initialization only once, but I think that's not worth it.

Ah, yes, you're right.
I missed the fact it's plain C, so a static variable must be initialized 
with a constant.

> Thanks,
> Volker
>
>
>> 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 :)
>>>
>