RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX

[Rob McKenna]

Hi Dmitry,

Thanks a lot for the comprehensive review. W.r.t. line 210, I agree 
there is a problem with the logic, but I'd like to suggest an 
alternative solution:

- If addrs4 >= 1, then we will always be including loopback addresses in 
the list. Since the idea of this extra condition is to exclude loopback 
interfaces from the list unless they're the only available addresses, I 
would suggest "(addrs4 == numV4Loopback && addrs6 == numV6Loopback)" 
instead.

- On the very limited chance that a user has a machine with only an ipv6 
loopback configured (unlikely, I'd agree) it probably makes sense to 
leave it in there. Actually, can you tell me why you'd rather not 
include ipv6 loopbacks at all?

New webrev at:

http://cr.openjdk.java.net/~robm/7180557/webrev.04/

     -Rob

On 21/09/13 12:20, Dmitry Samersoff wrote:
> Rob,
>
> See below -
> ll. 210 have to be fixed, other comments is just an opinion.
>
>
> Inet4AddressImpl.c:
>
> ll.132 - it might be better to move initialization to a separate
> function, the same way as in Inet6 -  I really like this idea.
>
> Inet6AddressImpl.c
>
>
> ll. 126.
>
> it's better to move static int initialized into initializeInetClasses()
> and don't check it ll. 282.
>
>
> ll. 170
>
> it looks like rest of the code uses NI_MAXHOST also we have to check
> results of JVM_GetHostName if it returns -1 it's probably better to
> bailout immediately.
>
>
> ll. 193 and below - wrong indent
>
> 4)
>
> ll. 210
>
> we can have more than one v4 address so
>
> if (addrs4 >= 1)
>
> have to be here.
>
> *.
>
> Your include ipv6 loopback in the list if interface has no ipv4
> addresses, I'm not sure this logic is correct. On my opinion it's better
> to never include ipv6 loopbacks.
>
> *.
>
> Is it better to rename:
> includeLoopback => includeLoopbacks
>
>
> ll. 218
>
> It might be better to calculate arraySize under if at ll. 210 to make
> code better readable
>
> ll. 236
>
> It might be better to split if statement to make it more readable at the
> cost of duplicating  iter = iter->ifa_next; line.
>
> e.g.
>
> while (iter != NULL) {
> ...
>
>    if (family != AF_INET6 and family != AF_INET) {
>      iter = iter->ifa_next;
>      continue;
>    }
>
>    if ((!includeV6 and family == AF_INET6)) {
>      iter = iter->ifa_next;
>      continue;
>    }
>
>    if (!includeLoopback and isLoopback) {
>      iter = iter->ifa_next;
>      continue;
>    }
>
>    if (iter->ifa_name[0] != '\0'  &&  iter->ifa_addr) {
>     // Copy address to Java array
>      ....
>      iter = iter->ifa_next;
>      continue; // redundant just for readability
>    }
> }
>
> ll.244
>
> I'm not sure it's good to return partially complete array in case of
> object allocation fail. Probably you should throw
>
> JNU_ThrowOutOfMemoryError(env, "Object allocation failed");
>
> -Dmitry
>
>
> On 2013-09-20 18:58, Rob McKenna wrote:
>> After a brief discussion with Chris, we decided to revert the position
>> of the call to lookupIfLocalAddrs so as to give the local host primacy
>> over DNS.
>>
>> Latest (and hopefully last) webrev here:
>>
>> http://cr.openjdk.java.net/~robm/7180557/webrev.03/
>>
>>      -Rob
>>
>> On 14/09/13 00:00, Rob McKenna wrote:
>>> Hi Bernd,
>>>
>>> I should have said in the context of this bug. What I meant was
>>> removing AI_CANONNAME doesn't resolve the issue as far as Mac is
>>> concerned. I.e. I still see the UnknownHostException. In this
>>> particular case the hostname is not set via the hosts file.
>>>
>>> In the latest webrev the call to getifaddrs only occurs if getaddrinfo
>>> fails.
>>>
>>>      -Rob
>>>
>>> On 13/09/13 20:28, Bernd Eckenfels wrote:
>>>> Am 13.09.2013, 19:32 Uhr, schrieb Rob McKenna <rob.mckenna at oracle.com>:
>>>>> W.r.t. the use of AI_CANONNAME, this doesn't actually make a
>>>>> difference in the context of this fix, but is definitely something
>>>>> that should be looked at. I'll put it on the todo list.
>>>> I think it does make a difference: If you remove the CANON flag
>>>> getaddrinfo() will not do DNS lookups when the host is configured to
>>>> prefer the hosts file (which it should do on Linux and OSX). And so
>>>> the platform specific lookupIfLocalhost() can be put after the
>>>> getaddrinfo() (again).
>>>>
>>>> I actually think the bug "exists" on all platforms. If getaddrinfo()
>>>> fails because neighter hosts nor DNS file finds the name this can
>>>> happen on all platforms. I dont think it helps to add a fallback only
>>>> on MACOSX (and there is certainly no need to prefer the fallback then).
>>>>
>>>> Gruss
>>>> Bernd
>