Rob,
Existing code uses if (JVM_GetHostName(myhostname, NI_MAXHOST)) so I
withdraw my last comments. Please, don't change it!!!
-Dmitry
On 2013-10-07 20:30, Rob McKenna wrote:
> 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.mcke...@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
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
