Skip to content
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

LibIPC: Port to Windows #2643

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

stasoid
Copy link
Contributor

@stasoid stasoid commented Nov 29, 2024

Requires #2673 #2674 #2946

@stasoid stasoid force-pushed the ipc branch 6 times, most recently from d46eb67 to 5f6ecf9 Compare November 30, 2024 12:50
@stasoid stasoid force-pushed the ipc branch 2 times, most recently from 365b491 to f9b95b6 Compare December 7, 2024 03:15
Libraries/LibIPC/File.cpp Show resolved Hide resolved
Libraries/LibIPC/Message.h Show resolved Hide resolved
Libraries/LibIPC/Connection.h Show resolved Hide resolved
@stasoid stasoid force-pushed the ipc branch 2 times, most recently from 4f57a24 to 70a8093 Compare December 7, 2024 08:30
@stasoid stasoid marked this pull request as ready for review December 7, 2024 08:32
@stasoid stasoid marked this pull request as draft December 7, 2024 09:28
@stasoid stasoid force-pushed the ipc branch 4 times, most recently from 30713c0 to 89d6a38 Compare December 9, 2024 08:04
@@ -27,54 +27,45 @@ namespace IPC::Concepts {

namespace Detail {

// Cannot use SpecializationOf with these templates because they have non-type parameters. See https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1985r3.pdf
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the order of declarations, which makes the diff unreadable. This is a diff that shows changes of substance (without reordering or inline removing).

@stasoid stasoid marked this pull request as ready for review December 9, 2024 13:09
@stasoid
Copy link
Contributor Author

stasoid commented Dec 9, 2024

@R-Goc @konradekk Please don't comment on my PRs. I am only interested in reviews from maintainers. So far your comments provided very little value for me.

I thought I can just ignore your comments, but Jelle Raaijmakers says unresolved comments may be the reason why my PR is not getting a review from maintainers, so I am asking not to spam here.

I am marking all conversations as resolved now to increase my chances for a review from maintainers.

@stasoid
Copy link
Contributor Author

stasoid commented Dec 17, 2024

I found a problem with the handling of sockets. When given a socket fd, _close does not call closesocket, it calls CloseHandle. But the doc for CloseHandle specifically says that CloseHandle should not be used for sockets. So WIN32 APIs behave inconsistently here. If _close cannot properly handle socket fds why _open_osfhandle works for socket handles? It should fail as it does for file mapping handles.

This cannot be fixed just by adding if (is_socket(fd)) closesocket(_get_osfhandle(fd)); to System::close because fd might be created with _dup, and, given above, I presume _dup doesn't properly duplicate sockets either (it is likely just duplicates the handles, not additional socket state).

So I am marking this as draft for now.

@stasoid
Copy link
Contributor Author

stasoid commented Dec 17, 2024

To fix the problem above, I now treat socket handles the same as file mapping handles. I.e. using pseudo file descriptor instead of calling _open_osfhandle, see #2946.

@stasoid stasoid marked this pull request as ready for review December 17, 2024 11:28
@stasoid stasoid force-pushed the ipc branch 2 times, most recently from 615c9a8 to c3a5079 Compare December 17, 2024 13:46
@stasoid stasoid marked this pull request as draft December 26, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants