Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
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
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
Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
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
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
