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

Replace socket2 calls with rustix #146

Merged
merged 7 commits into from
Oct 8, 2023
Merged

Replace socket2 calls with rustix #146

merged 7 commits into from
Oct 8, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Sep 7, 2023

socket2 is our last libc-using dependency. By replacing it with rustix calls, we can make this crate libc-free.

This doesn't set the inherit disable property on Windows yet, as rustix does not support it.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +2014 to +2107
// Set nosigpipe if necessary.
#[cfg(any(
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
target_os = "freebsd"
))]
Copy link
Collaborator

@taiki-e taiki-e Sep 7, 2023

Choose a reason for hiding this comment

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

AFAIK, NetBSD also supports nosigpipe (OpenBSD and DragonflyBSD do not).

Also, do you know why socket2 sets nosigpipe only on Apple targets, not also on FreeBSD/NetBSD?
https://github.com/rust-lang/socket2/blob/66ed6b055a4352a26ed25e8d01981fb5b555f3d8/src/socket.rs#L798
Maybe because MSG_NOSIGNAL has already been passed?
https://github.com/rust-lang/rust/blob/fece303aeb1d2b845745bc3c067475619670a868/library/std/src/sys_common/net.rs#L38

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I would bet that it's because FreeBSD/NetBSD did not have it at the time.

Copy link
Collaborator

@taiki-e taiki-e Oct 8, 2023

Choose a reason for hiding this comment

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

Opened bytecodealliance/rustix#870 for NetBSD and DragonFly BSD.

@taiki-e
Copy link
Collaborator

taiki-e commented Sep 7, 2023

This doesn't set the inherit disable property on Windows yet, as rustix does not support it.

I think we can set HANDLE_FLAG_INHERIT by using SetHandleInformation from windows-sys for now. (We already depend on windows-sys via polling and rustix.) Ideally, I would like to set that at the time the socket is created (i.e., by using SocketFlags).

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
socket2 is our last libc-using dependency. By replacing it with rustix
calls, we can make this crate libc-free.

This doesn't set the inherit disable property on Windows yet, as rustix
does not support it.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Oct 7, 2023

The OpenBSD test failure is rust-lang/rust#116523 combined with bytecodealliance/rustix#869

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Oct 8, 2023

I've worked around this issue by disabling uds_socket on OpenBSD for now.

@notgull notgull merged commit 0b5016e into master Oct 8, 2023
21 checks passed
@notgull notgull deleted the notgull/no-socket2 branch October 8, 2023 04:18
notgull added a commit that referenced this pull request Dec 6, 2023
In #146, I introduced a bug that prevented abstract sockets from
working. I passed the path straight into
rustix::net::SocketAddrUnix::new, which fails if it receives an abstract
socket.

This commit fixes this issue by explicitly checking for abstract
sockets. If it sees that the path it's receiving is abstract, it will
pass the path's bytes to new_abstract_socket() instead.

This should fix the issue that is occurring in dbus2/zbus#517

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this pull request Dec 7, 2023
In #146, I introduced a bug that prevented abstract sockets from
working. I passed the path straight into
rustix::net::SocketAddrUnix::new, which fails if it receives an abstract
socket.

This commit fixes this issue by explicitly checking for abstract
sockets. If it sees that the path it's receiving is abstract, it will
pass the path's bytes to new_abstract_socket() instead.

This should fix the issue that is occurring in dbus2/zbus#517

Signed-off-by: John Nunley <dev@notgull.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants