-
Notifications
You must be signed in to change notification settings - Fork 160
8339414: Fix JDK-8202369 incorrect backport for 8u #661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back fitzsim! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this seems like a clear case of the duplication of getLocalHostName(JNIEnv *env, jobject this)
being missed in backporting from 11u (which has a much simpler addition of the #ifdef
and the #else
case). There's a lot of noise in removing the stored error
value both in the original 8u backport and this change, but the overall change amounts to the same as 11u.
To that end, I don't think a partial backport of JDK-8202181 belongs here. Even if we agree not to alter the branch without HAS_GLIBC_GETHOSTBY_R
(which I think has merit as we don't define NI_MAXHOST
ourselves there), we should fix getHostByAddr
and the IPv6 implementation. So I'd rather see that handled in full as a separate PR, rather than deviating from the 8202369 backport here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Happy for this to go in now it's just the backport.
Please apply for approval
|
/approval request This fix is ready. |
There is a large
#ifdef
in JDK 8'sInet4AddressImpl.c
:When the fix for https://bugs.openjdk.org/browse/JDK-8201369 was backported to
8
, the second branch of the large#ifdef
inInet4AddressImpl.c
was not changed at the same time, which is incorrect, since reverse lookup should only ever happen onSolaris
, according to the description ofJDK-8201369
.I used the
JDK 11
unix/native/libnet/Inet4AddressImpl.c
implementation; after this change, the only difference between11
'sJava_java_net_Inet4AddressImpl_getLocalHostName
and thedefined(HAS_GLIBC_GETHOSTBY_R)
one will be the use ofJVM_GetHostName
in8
versusgethostname
in11
.I decided to update the
hostlen
argument togetnameinfo
fromNI_MAXHOST
tosizeof(hostname)
to fix https://bugs.openjdk.org/browse/JDK-8202181 for this function. I do not have aSolaris
machine to test on, but this matches11
'sgetnameinfo
calls, and is correct givenhostname[NI_MAXHOST] = '\0'
.The resulting behaviour should be the same, other than the reverse lookup only being performed on
Solaris
.The implementations of
Java_java_net_Inet4AddressImpl_getLocalHostName
in each of the#ifdef
branches diverge already because the fix for https://bugs.openjdk.org/browse/JDK-7112670 (a62dd62) was only applied to the second branch.Probably the upper
Java_java_net_Inet4AddressImpl_getLocalHostName
implementation could be made identical to the lower, but I do not want to do that, just in caseNI_MAXHOST
is preferred tosizeof(hostname)
on!defined(HAS_GLIBC_GETHOSTBY_R)
operating systems.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/661/head:pull/661
$ git checkout pull/661
Update a local copy of the PR:
$ git checkout pull/661
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/661/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 661
View PR using the GUI difftool:
$ git pr show -t 661
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/661.diff
Using Webrev
Link to Webrev Comment