Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Heh, you just beat me to the punch.
-Rob
On 07/10/13 17:34, Dmitry Samersoff wrote:
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
:
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 i
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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
:
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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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
> :
>> 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 f
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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 :
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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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 :
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.
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Looks fine to me Rob, thanks. -Chris. On 09/20/2013 03:58 PM, 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 : 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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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 : 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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
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 : 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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Am 13.09.2013, 19:32 Uhr, schrieb Rob McKenna : 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
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Hi folks, updated webrev at: http://cr.openjdk.java.net/~robm/7180557/webrev.02/ Hopefully all of your concerns have been addressed. 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. -Rob On 05/09/13 21:33, Dmitry Samersoff wrote: Rob, Did you try to remove hints.ai_flags = AI_CANONNAME this flag asks getaddreinfo to return FQDN, but the function behavior is not clearly defined for the case where FQDN is not available. -Dmitry On 2013-09-03 19:18, Rob McKenna wrote: Hi folks, Mac seems to have trouble looking up local hostnames using getaddrinfo unless a domain is set. The solution is to add a check with getifaddrs . This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen a canonical answer on whether this is the way to go so I figured trial by fire might get the discussion going. http://cr.openjdk.java.net/~robm/7180557/webrev.01/ -Rob
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Thanks for the comments folks. Chris, I like the idea of moving this check below the GAI call. Dmitry, that loop is indeed a bit of a code smell. I'll take care of it. Bernd / Dmitry, thanks for the notes on AI_CANONNAME. I'll adjust the code and get some testing done then report back! I've kept this code as close to the apple version as sensible but the feedback received so far trumps that. -Rob On 05/09/13 22:30, Bernd Eckenfels wrote: Hello, I reported before, AI_CANONNAME is used in different places with no good reason. If you use the flag, the result would be in res[0].ai_canonname, which is not used. So you can remove it and safe the elaborate resolving which comes with it. And I also think the comment "skip DNS lookup" is wrong, as GAI typically (also) looks in /etc/hosts - and this is also documented in the mac os man page. So I think if you remove the flag you dont need the lookupIflocalhost shortcut at all (or you need it on all systems). And if you have a look there, I think the AI_ADDRCONFIG should be considered, instead. Gruss Bernd Am 05.09.2013, 22:33 Uhr, schrieb Dmitry Samersoff : Rob, Did you try to remove hints.ai_flags = AI_CANONNAME this flag asks getaddreinfo to return FQDN, but the function behavior is not clearly defined for the case where FQDN is not available.
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob, Instead of iterating interfaces list twice you can maintain two indices, as you already know exact number of addresses. put_v4_at(i) put_v6_at(j+v4count) Also you can take a look at solaris/native/java/net/NetworkInterface.c and possibly re-use some logic from there. -Dmitry On 2013-09-03 19:18, Rob McKenna wrote: > Hi folks, > > Mac seems to have trouble looking up local hostnames using getaddrinfo > unless a domain is set. The solution is to add a check with getifaddrs . > > This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen > a canonical answer on whether this is the way to go so I figured trial > by fire might get the discussion going. > > http://cr.openjdk.java.net/~robm/7180557/webrev.01/ > > -Rob > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Hello, I reported before, AI_CANONNAME is used in different places with no good reason. If you use the flag, the result would be in res[0].ai_canonname, which is not used. So you can remove it and safe the elaborate resolving which comes with it. And I also think the comment "skip DNS lookup" is wrong, as GAI typically (also) looks in /etc/hosts - and this is also documented in the mac os man page. So I think if you remove the flag you dont need the lookupIflocalhost shortcut at all (or you need it on all systems). And if you have a look there, I think the AI_ADDRCONFIG should be considered, instead. Gruss Bernd Am 05.09.2013, 22:33 Uhr, schrieb Dmitry Samersoff : Rob, Did you try to remove hints.ai_flags = AI_CANONNAME this flag asks getaddreinfo to return FQDN, but the function behavior is not clearly defined for the case where FQDN is not available.
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob, Did you try to remove hints.ai_flags = AI_CANONNAME this flag asks getaddreinfo to return FQDN, but the function behavior is not clearly defined for the case where FQDN is not available. -Dmitry On 2013-09-03 19:18, Rob McKenna wrote: > Hi folks, > > Mac seems to have trouble looking up local hostnames using getaddrinfo > unless a domain is set. The solution is to add a check with getifaddrs . > > This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen > a canonical answer on whether this is the way to go so I figured trial > by fire might get the discussion going. > > http://cr.openjdk.java.net/~robm/7180557/webrev.01/ > > -Rob > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
On 09/04/2013 02:49 PM, Rob McKenna wrote: Indeed it did. Great, so we should behave in a similar manner to that of Apple's JDK6. I think I understand why you added lookupIfLocalhost before getaddrinfo, in lookupAllHostAddr, but it has the consequence of retrieving the hostname for every other lookup. I wonder if the new code could be added after getaddrinfo, if no address could be found? This would mean that a DNS name would trump the local machine configuration, but that may not be so bad. -Chris. -Rob On 04/09/13 10:36, Chris Hegarty wrote: Rob, I haven't looked at the changes yet, but I'm curious about the decision to use getifaddrs. I know that there was a discussion started at one point to determine what Apple's JDK6 is doing to retrieve the local machines IP information. Did that discussion lead to getifaddrs? -Chris. On 03/09/2013 16:18, Rob McKenna wrote: Hi folks, Mac seems to have trouble looking up local hostnames using getaddrinfo unless a domain is set. The solution is to add a check with getifaddrs . This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen a canonical answer on whether this is the way to go so I figured trial by fire might get the discussion going. http://cr.openjdk.java.net/~robm/7180557/webrev.01/ -Rob
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Indeed it did. -Rob On 04/09/13 10:36, Chris Hegarty wrote: Rob, I haven't looked at the changes yet, but I'm curious about the decision to use getifaddrs. I know that there was a discussion started at one point to determine what Apple's JDK6 is doing to retrieve the local machines IP information. Did that discussion lead to getifaddrs? -Chris. On 03/09/2013 16:18, Rob McKenna wrote: Hi folks, Mac seems to have trouble looking up local hostnames using getaddrinfo unless a domain is set. The solution is to add a check with getifaddrs . This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen a canonical answer on whether this is the way to go so I figured trial by fire might get the discussion going. http://cr.openjdk.java.net/~robm/7180557/webrev.01/ -Rob
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob, I haven't looked at the changes yet, but I'm curious about the decision to use getifaddrs. I know that there was a discussion started at one point to determine what Apple's JDK6 is doing to retrieve the local machines IP information. Did that discussion lead to getifaddrs? -Chris. On 03/09/2013 16:18, Rob McKenna wrote: Hi folks, Mac seems to have trouble looking up local hostnames using getaddrinfo unless a domain is set. The solution is to add a check with getifaddrs . This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen a canonical answer on whether this is the way to go so I figured trial by fire might get the discussion going. http://cr.openjdk.java.net/~robm/7180557/webrev.01/ -Rob
RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Hi folks, Mac seems to have trouble looking up local hostnames using getaddrinfo unless a domain is set. The solution is to add a check with getifaddrs . This fix replaces a usage of _ALLBSD_SOURCE with MACOSX. I haven't seen a canonical answer on whether this is the way to go so I figured trial by fire might get the discussion going. http://cr.openjdk.java.net/~robm/7180557/webrev.01/ -Rob
