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

[Rob McKenna]

Thanks Dmitry! I'll correct that nipick pre-push.

     -Rob

On 07/10/13 16:47, Dmitry Samersoff wrote:
> Rob,
>
> This version of your fix looks good for me.
>
>
> Inet4AddressImpl.c:
>
>    Thumbs up.
>
> Inet6AddressImpl.c:
>
>    Thumbs up.
>
> 173     if (JVM_GetHostName(myhostname, NI_MAXHOST)) {
>
> Nitpicking, explicit == -1 would be better here.
>
>
>> Actually, can you tell me why you'd rather not
>> include ipv6 loopbacks at all?
> If interface don't have ipv6 address but we have ipv6 loopback it most
> likely means that ipv6 is not functioning properly.
>
> -Dmitry
>
>
> On 2013-10-04 19:03, Rob McKenna wrote:
>> 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
>