8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options #10278

caoman · 2022-09-15T07:25:04Z

Hi all,

Could anyone review this bug fix for ipv6-only system? See https://bugs.openjdk.org/browse/JDK-8293842 for detailed description of this bug.

Ideally, the socket(PF_INET6, ...) or socket(PF_INET, ...) call should depend on the value of ipv6_available(). However, this is only easy to do in net_util_md.c. Without checking ipv6_available() in <Linux|MacOSX>SocketOptions.c, I'm not sure if it is possible for ipv6 and ipv4 sockets to differ in the options they support.

For example, for a system with both ipv6 and ipv4, if ipv6 supports TCP_KEEPIDLE but ipv4 does not, then there might be a problem to always report TCP_KEEPIDLE as supported.

I noticed https://bugs.openjdk.org/browse/JDK-8290349 added "isIPv6" parameter to a few functions in <Linux|MacOSX>SocketOptions.c. However, it is not applicable to the socketOptionSupported() function. This change is more similar to the Java_jdk_net_MacOSXSocketOptions_ipDontFragmentSupported0() function.

-Man

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options

Reviewers

Daniel Jeliński (@djelinski - Committer) ⚠️ Review applies to 6a2c295b
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 6a2c295b

Contributors

Martin Buchholz <martin@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10278/head:pull/10278
$ git checkout pull/10278

Update a local copy of the PR:
$ git checkout pull/10278
$ git pull https://git.openjdk.org/jdk pull/10278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10278

View PR using the GUI difftool:
$ git pr show -t 10278

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10278.diff

…everal socket/TCP options

bridgekeeper · 2022-09-15T07:26:57Z

👋 Welcome back manc! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-09-15T07:28:56Z

@caoman The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-09-15T07:33:26Z

Webrevs

01: Full - Incremental (64d767f4)
00: Full (6a2c295b)

djelinski

Other than the indentation, LGTM. Thanks!

src/java.base/unix/native/libnet/net_util_md.c

src/java.base/share/native/libnet/net_util.c

dfuch · 2022-09-15T18:21:17Z

The proposed changes look reasonable. I have imported the PR and run tier2 test and observed no regressions.

openjdk · 2022-09-15T18:23:14Z

@caoman 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:

8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options

Co-authored-by: Martin Buchholz <martin@openjdk.org>
Reviewed-by: djelinski, dfuchs

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 13 new commits pushed to the master branch:

bb9aa4e: 8293813: ProblemList com/sun/jdi/JdbLastErrorTest.java on windows-x64 in Xcomp mode
4cec141: 8291509: Minor cleanup could be done in sun.security
6beeb84: 8293875: ProblemList sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 on linux-x64
bf79f99: 8292989: Avoid dynamic memory in AsyncLogWriter
2028ec7: 8289608: Change com/sun/jdi tests to not use Thread.suspend/resume
ecb456a: 8293779: redundant checking in AESCrypt.makeSessionKey() method
6fca9ae: 8288474: Move EventContinuationFreezeOld from try_freeze_fast to freeze_slow
fbd8b42: 8293591: Remove use of Thread.stop from jshell tests
aff5ff1: 8244681: Add a warning for possibly lossy conversion in compound assignments
15cb1fb: 8256265: G1: Improve parallelism in regions that failed evacuation
... and 3 more: https://git.openjdk.org/jdk/compare/eeb625e7095e65e64023cbfe05e579af90f4b638...master

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 master branch, type /integrate in a new comment.

caoman · 2022-09-15T19:01:05Z

Thank you all for the quick review and feedback!

caoman · 2022-09-15T21:15:42Z

/contributor add martin

Martin initially implemented this fix a while back.

openjdk · 2022-09-15T21:16:05Z

@caoman
Contributor Martin Buchholz <martin@openjdk.org> successfully added.

caoman · 2022-09-15T22:05:10Z

/integrate

openjdk · 2022-09-15T22:06:33Z

Going to push as commit 9a40b76.
Since your change was applied there have been 13 commits pushed to the master branch:

bb9aa4e: 8293813: ProblemList com/sun/jdi/JdbLastErrorTest.java on windows-x64 in Xcomp mode
4cec141: 8291509: Minor cleanup could be done in sun.security
6beeb84: 8293875: ProblemList sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 on linux-x64
bf79f99: 8292989: Avoid dynamic memory in AsyncLogWriter
2028ec7: 8289608: Change com/sun/jdi tests to not use Thread.suspend/resume
ecb456a: 8293779: redundant checking in AESCrypt.makeSessionKey() method
6fca9ae: 8288474: Move EventContinuationFreezeOld from try_freeze_fast to freeze_slow
fbd8b42: 8293591: Remove use of Thread.stop from jshell tests
aff5ff1: 8244681: Add a warning for possibly lossy conversion in compound assignments
15cb1fb: 8256265: G1: Improve parallelism in regions that failed evacuation
... and 3 more: https://git.openjdk.org/jdk/compare/eeb625e7095e65e64023cbfe05e579af90f4b638...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-15T22:06:57Z

@caoman Pushed as commit 9a40b76.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

AlanBateman · 2022-09-16T06:34:11Z

/integrate

The changes that you integrated aren't the same as the changes that were reviewed. What you have is okay but if you make changes in response to reviewer feedback then you should really wait to see if there are comments on the changes.

caoman · 2022-09-16T21:06:40Z

/integrate

The changes that you integrated aren't the same as the changes that were reviewed. What you have is okay but if you make changes in response to reviewer feedback then you should really wait to see if there are comments on the changes.

I see. Apology for that. I'll make sure to wait for approvals for the latest commit in the future.

8293842: IPv6-only systems throws UnsupportedOperationException for s…

6a2c295

…everal socket/TCP options

openjdk bot added the rfr Pull request is ready for review label Sep 15, 2022

openjdk bot added the net net-dev@openjdk.org label Sep 15, 2022

djelinski approved these changes Sep 15, 2022

View reviewed changes

src/java.base/unix/native/libnet/net_util_md.c Outdated Show resolved Hide resolved

AlanBateman reviewed Sep 15, 2022

View reviewed changes

src/java.base/share/native/libnet/net_util.c Outdated Show resolved Hide resolved

dfuch approved these changes Sep 15, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 15, 2022

Address comments

64d767f

openjdk bot added the integrated Pull request has been integrated label Sep 15, 2022

openjdk bot closed this Sep 15, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2022

caoman deleted the ipv6options branch September 15, 2022 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options #10278

8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options #10278

caoman commented Sep 15, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

mlbridge bot commented Sep 15, 2022 •

edited

Loading

djelinski left a comment

dfuch commented Sep 15, 2022

openjdk bot commented Sep 15, 2022 •

edited

Loading

caoman commented Sep 15, 2022

caoman commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

caoman commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

AlanBateman commented Sep 16, 2022

caoman commented Sep 16, 2022

8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options #10278

8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options #10278

Conversation

caoman commented Sep 15, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

mlbridge bot commented Sep 15, 2022 • edited Loading

Webrevs

djelinski left a comment

Choose a reason for hiding this comment

dfuch commented Sep 15, 2022

openjdk bot commented Sep 15, 2022 • edited Loading

caoman commented Sep 15, 2022

caoman commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

caoman commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

AlanBateman commented Sep 16, 2022

caoman commented Sep 16, 2022

caoman commented Sep 15, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 15, 2022 •

edited

Loading

openjdk bot commented Sep 15, 2022 •

edited

Loading