Simplify Windows debugger socket setup #10212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Don't use SO_OPENTYPE in Windows debugger. It was probably deprecated at some point, and the same feature can be achieved with WSASocket. The documentation reads (about SO_OPENTYPE): > Once set, affects whether subsequent sockets that are created will > be non-overlapped. The possible values for this option are > SO_SYNCHRONOUS_ALERT and SO_SYNCHRONOUS_NONALERT. This option should > not be used. Instead use the WSASocket function and leave the > WSA_FLAG_OVERLAPPED bit in the dwFlags parameter turned off. https://docs.microsoft.com/en-us/windows/win32/winsock/sol-socket-socket-options This allows to merge Windows-specific code and to remove all the sockopt related code. Keeping the _open_osfhandle error is not necessary. No change entry needed
MisterDA
force-pushed
the
debugger-winsock2
branch
from
ae2b458
to
e13bd18
Compare
xavierleroy
approved these changes
Feb 16, 2021
There was a problem hiding this comment.
Looks good to me. WSAsocket was introduced in Vista and Server 2003, after the original code was written. It's probably OK to use it, provided we update the minimal Windows requirements in README.win32.doc.
Error reporting is not quite right, but it's no worse than in the old code, see comment below.
|
I took the liberty to push a tentative improvement to your branch. Please comment! And let's see what CI says about it. |
MisterDA
commented
Feb 16, 2021
|
@MisterDA and anyone else who would care to comment: are you OK with my proposed change to error handling? If so I'll merge this PR. |
|
I'm ok with the proposed change. |
|
Precheck CI was positive. I added a Changes entry and merged. |
garrigue
pushed a commit
to garrigue/ocaml
that referenced
this pull request
Mar 3, 2021
Don't use SO_OPENTYPE in Windows debugger. It was probably deprecated at some point, and the same feature can be achieved with WSASocket. The documentation reads (about SO_OPENTYPE): > Once set, affects whether subsequent sockets that are created will > be non-overlapped. The possible values for this option are > SO_SYNCHRONOUS_ALERT and SO_SYNCHRONOUS_NONALERT. This option should > not be used. Instead use the WSASocket function and leave the > WSA_FLAG_OVERLAPPED bit in the dwFlags parameter turned off. https://docs.microsoft.com/en-us/windows/win32/winsock/sol-socket-socket-options This allows to merge Windows-specific code and to remove all the sockopt related code. Keeping the _open_osfhandle error is not necessary. Also: improve "cannot connect" error reporting under Windows Co-authored-by: Xavier Leroy <xavier.leroy@college-de-france.fr>
smuenzel
pushed a commit
to smuenzel/ocaml
that referenced
this pull request
Mar 30, 2021
Don't use SO_OPENTYPE in Windows debugger. It was probably deprecated at some point, and the same feature can be achieved with WSASocket. The documentation reads (about SO_OPENTYPE): > Once set, affects whether subsequent sockets that are created will > be non-overlapped. The possible values for this option are > SO_SYNCHRONOUS_ALERT and SO_SYNCHRONOUS_NONALERT. This option should > not be used. Instead use the WSASocket function and leave the > WSA_FLAG_OVERLAPPED bit in the dwFlags parameter turned off. https://docs.microsoft.com/en-us/windows/win32/winsock/sol-socket-socket-options This allows to merge Windows-specific code and to remove all the sockopt related code. Keeping the _open_osfhandle error is not necessary. Also: improve "cannot connect" error reporting under Windows Co-authored-by: Xavier Leroy <xavier.leroy@college-de-france.fr>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't use
SO_OPENTYPEin Windows debugger. It was probably deprecatedat some point, and the same feature can be achieved with
WSASocket.The documentation reads (about
SO_OPENTYPE):https://docs.microsoft.com/en-us/windows/win32/winsock/sol-socket-socket-options
This allows to merge Windows-specific code and to remove all the
sockopt related code. Keeping the
_open_osfhandleerror is notnecessary.