-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8373676: Test javax/net/ssl/HttpsURLConnection/SubjectAltNameIP.java fails on a machine without IPV6 #28825
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
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
|
@MBaesken This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 29 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| if (ipv6av) { | ||
| JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", "Protocol family ipv6, ipv6 on machine available"); |
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.
This exception does not really make sense. Are you really sure it's possible to get 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.
Looks like to get into the else case starting at line 284 or before the patch like 282 , ipv6_available() / ipv6av can be false ; or also the second part of the if check can be false
!(family == java_net_InetAddress_IPv4 && v4MappedAddress == JNI_FALSE))
so we should distinguish the 2 cases. Not sure if this really happens, on our system showing the error we got the other one Caused by: java.net.SocketException: Protocol family ipv6, ipv6 on machine unavailable
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.
Or just write the old message "Protocol family unavailable" for this case and enhance the else case ?
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.
Looking more at the checks, I think we can always use the exception
JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", "Protocol family ipv6, ipv6 on machine unavailable");
without the if and else. At least as long as the other if checks do not change. Do you agree?
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.
That's what I would expect. I have not double checked.
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.
I believe you are right - we enter the if part if:
ipv6 is available AND ( family != IPv4 OR mapped == true)
=> we either have an IPv6 address or we have an IPv4 address that will be mapped to an IPv6 address
So if we enter the else, either we have an IPv4 address, or we don't. If we don't, then we are here because IPv6 is not available.
|
|
||
| /* | ||
| * @test | ||
| * @bug 8369950 |
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.
nit: could you please add this bug id to the @bug annotation 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.
I don't think that's useful. The implementation changes are unrelated. It could be better to mark the JBS issue with one of the noreg labels.
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.
Maybe it would be best to split out the exception message improvement in a new JBS issue.
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.
Maybe it would be best to split out the exception message improvement in a new JBS issue.
Sure , we can do this.
I created https://bugs.openjdk.org/browse/JDK-8373704
Improve SocketException: Protocol family unavailable
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.
Thank you Matthias!
|
I removed the net_util_md.c exception message changes. |
jaikiran
left a comment
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.
The change looks OK to me.
It's a bit odd that this sun.net.util.IPAddressUtil.isIPv6LiteralAddress("[::1]") returns false where as InetAddress.ofLiteral("[::1]") returns a valid InetAddress. But IPAddressUtil is an internal implementation util, so that's what the expectation maybe of that implementation. The change to this test looks OK to me.
FWIW |
dfuch
left a comment
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.
LGTM
|
Thanks for the reviews ! Maybe someone wants to look also at https://bugs.openjdk.org/browse/JDK-8373704 / #28851 ? |
|
/integrate |
|
Going to push as commit 9e2008b.
Your commit was automatically rebased without conflicts. |
We have a Linux machine with IPV6 disabled.
There the test javax/net/ssl/HttpsURLConnection/SubjectAltNameIP.java fails.
Error is
After looking into the test, it turned out the IPV6 address
::1was passed in the notion[::1]to the isIPv6LiteralAddress, but this method must get the address without '[' and ']' .Additionally I adjusted the exception a bit so that it directly mentions IPV6 and not just some 'protol family' .
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28825/head:pull/28825$ git checkout pull/28825Update a local copy of the PR:
$ git checkout pull/28825$ git pull https://git.openjdk.org/jdk.git pull/28825/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28825View PR using the GUI difftool:
$ git pr show -t 28825Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28825.diff
Using Webrev
Link to Webrev Comment