Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
Hi,
under the following link you can find an updated version of my change
where I've fixed all indentation in Inet{4,6}AddressImpl.c to 4 spaces
and changed the few C++-style comments (i.e. '//') to C-style comments
(i.e. '/* */'):
http://cr.openjdk.java.net/~simonis/webrevs/8060470.v2/
Can I please also get a review with regards to the content of this
change (as opposed to its form:)
Thank you and best regards,
Volker
On Mon, Oct 27, 2014 at 7:27 PM, Alan Bateman alan.bate...@oracle.com wrote:
On 27/10/2014 17:45, Volker Simonis wrote:
:
Well, the problem is that already that very file contains code in both
code conventions (see for example the implementations of 'ping4()'
and 'Java_java_net_Inet4AddressImpl_isReachable0()' in
Inet4AddressImpl.c which are mostly indented by two spaces). I have no
problems to adhere to any convention as long as it is generally
obeyed. As this does not seemed to be the case in these files, I've
just chosen what I thought is most appropriate. So to keep a long
story short - I can either:
1. indent my changes to four spaces (which will still let the files
with mixed indentation)
2. change all indentation in the file to two spaces
3. change all indentation in the file to four spaces
Please just tell me what you'd prefer.
We normally use 4 space indent in the library code. There are some
inconsistencies, I don't know the fully history to understand how we got
2-space indent but if you can fix it up then that would be great.
-Alan
Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
Hello,
I (still) like it. But I still think the AI_CANONNAME can and should be
removed. If you keep it, it will trigger additional lookups and the
actual canonized result (res-ai_canonname) is never used.
This is true for all 3 locations (but you changed only one).
I wonder if you want to make the control flags c-static and avoid the
lookups.
Gruss
Bernd
Am Fri, 31 Oct 2014 15:35:51 +0100
schrieb Volker Simonis volker.simo...@gmail.com:
Hi,
under the following link you can find an updated version of my change
where I've fixed all indentation in Inet{4,6}AddressImpl.c to 4 spaces
and changed the few C++-style comments (i.e. '//') to C-style comments
(i.e. '/* */'):
http://cr.openjdk.java.net/~simonis/webrevs/8060470.v2/
Can I please also get a review with regards to the content of this
change (as opposed to its form:)
Thank you and best regards,
Volker
On Mon, Oct 27, 2014 at 7:27 PM, Alan Bateman
alan.bate...@oracle.com wrote:
On 27/10/2014 17:45, Volker Simonis wrote:
:
Well, the problem is that already that very file contains code in
both code conventions (see for example the implementations of
'ping4()' and 'Java_java_net_Inet4AddressImpl_isReachable0()' in
Inet4AddressImpl.c which are mostly indented by two spaces). I
have no problems to adhere to any convention as long as it is
generally obeyed. As this does not seemed to be the case in these
files, I've just chosen what I thought is most appropriate. So to
keep a long story short - I can either:
1. indent my changes to four spaces (which will still let the
files with mixed indentation)
2. change all indentation in the file to two spaces
3. change all indentation in the file to four spaces
Please just tell me what you'd prefer.
We normally use 4 space indent in the library code. There are some
inconsistencies, I don't know the fully history to understand how
we got 2-space indent but if you can fix it up then that would be
great.
-Alan
Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
On 27/10/2014 17:45, Volker Simonis wrote: : Well, the problem is that already that very file contains code in both code conventions (see for example the implementations of 'ping4()' and 'Java_java_net_Inet4AddressImpl_isReachable0()' in Inet4AddressImpl.c which are mostly indented by two spaces). I have no problems to adhere to any convention as long as it is generally obeyed. As this does not seemed to be the case in these files, I've just chosen what I thought is most appropriate. So to keep a long story short - I can either: 1. indent my changes to four spaces (which will still let the files with mixed indentation) 2. change all indentation in the file to two spaces 3. change all indentation in the file to four spaces Please just tell me what you'd prefer. We normally use 4 space indent in the library code. There are some inconsistencies, I don't know the fully history to understand how we got 2-space indent but if you can fix it up then that would be great. -Alan
Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
On 27.10.2014 21:45, Volker Simonis wrote:
On Fri, Oct 24, 2014 at 7:07 PM, Ivan Gerasimov
ivan.gerasi...@oracle.com wrote:
Hi Volker!
I'm not a Reviewer, but have a couple of minor comments.
In the C source files you changed the indentation to two spaces.
It looks inconsistent with other JDK sources.
I know that in hotspot they use two space indentation, but it's a different
set of sources.
Well, the problem is that already that very file contains code in both
code conventions (see for example the implementations of 'ping4()'
and 'Java_java_net_Inet4AddressImpl_isReachable0()' in
Inet4AddressImpl.c which are mostly indented by two spaces). I have no
problems to adhere to any convention as long as it is generally
obeyed. As this does not seemed to be the case in these files, I've
just chosen what I thought is most appropriate. So to keep a long
story short - I can either:
1. indent my changes to four spaces (which will still let the files
with mixed indentation)
2. change all indentation in the file to two spaces
3. change all indentation in the file to four spaces
Please just tell me what you'd prefer.
I think #1 it the most appropriate here.
Inet4AddressImpl.c:
110 jboolean reverseLookup = (*env)-GetStaticBooleanField(env, ia_class,
ia_doIPv4ReverseLookup);
Since doIPv4ReverseLookup never changes, wouldn't it make sense to declare
jboolean reverseLookup static?
This way it would be retrieved only once.
The same in Inet6AddressImpl.c:
66 jboolean reverseLookup = (*env)-GetStaticBooleanField(env, ia_class,
ia_doIPv6ReverseLookup);
And a static value retrieved here:
68 if ((*env)-GetStaticBooleanField(env, ia_class,
ia_preferIPv6AddressID)) {
I think simply declaring the mentioned variables static is not
possible in C and this is a C file compiled with a C compiler.
You would get a initializer element is not constant error (see for
example
http://stackoverflow.com/questions/5921920/difference-between-initialization-of-static-variables-in-c-and-c).
I could of course use a second static varibale to do the
initialization only once, but I think that's not worth it.
Ah, yes, you're right.
I missed the fact it's plain C, so a static variable must be initialized
with a constant.
Thanks,
Volker
Sincerely yours,
Ivan
On 24.10.2014 18:47, Volker Simonis wrote:
Hi,
could somebody please have a quick look at this change.
It's really not that complicated as it looks like from the comments -
I just didn't manage to write it up in a more concise way :)
Thanks,
Volker
On Thu, Oct 16, 2014 at 4:39 PM, Volker Simonis
volker.simo...@gmail.com wrote:
Hi,
could you please hava a look at the following change:
http://cr.openjdk.java.net/~simonis/webrevs/8060470.v1
https://bugs.openjdk.java.net/browse/JDK-8060470
It's probably easier to read the following in the webrev, but I copy
it below for discussion.
Regards,
Volker
So here comes the first version of this change. Its main focus is the
unification and simplification of
Inet{4,6}AddressImpl_getLocalHostName() (notice that these native
methods are currently only called from InetAddress.getLocalHost() so
the impact is manageable):
- Simplification: the current implementation has three versions of
this function: two versions of Inet4AddressImpl_getLocalHostName()
(one for _ALLBSD_SOURCE !HAS_GLIBC_GETHOSTBY_R and another one
for the other Unix versions) and one version of
Inet6AddressImpl_getLocalHostName(). All these functions are very
similar and can be easily factored out into one new method.
- Unification: there are subtle and probably unnecessary differences
between the IPv4 and IPv6 version of these methods which can be easily
eliminated.
The only difference between the two IPv4 versions was the ai_family
flag passed as hint to the getaddrinfo() call. The Mac version used
AF_UNSPEC while the general Unix version used AF_INET. I don't see a
reason (and my tests didn't show any problems) why we couldn't use
AF_INET on MacOS as well.
The IPv6 version used AF_UNSPEC as well. The new refactored method
getLocalHostName() which is now called from both, the single instance
of Inet4AddressImpl_getLocalHostName() and
Inet6AddressImpl_getLocalHostName() uses AF_INET in the IPv4 case
(which shouldn't change anything) and AF_INET6 for the IPv6 case.
Additionally, it uses the flag AI_V4MAPPED in the IPv6 case. This will
return an IPv4-mapped IPv6 addresses if no matching IPv6 addresses
could be found.
The last difference between the old IPv4 and IPv6 versions was the
fact that the IPv4 versions always did a reverse lookup for the host
name. That means that after querying the hostname with gethostname(),
they used a call to getaddrinfo() to get the IP address of the host
name and finally they called getnameinfo() on that IP address to get
the host name once again. The IPv6 version only did this reverse
lookup on Solaris.
It is unclear why this reverse lookup was necessary at all. Especially
if we take into account that the
