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.
