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
<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
