Replace nix with rustix #815

notgull · 2023-04-28T23:47:02Z

Closes #770 by replacing nix with rustix. rustix has several advantages over nix, the prime one being that, on Linux, system calls are done without going through the libc. This means that, in certain cases, the system calls can be directly inlined into the functioned that call them.

The only issue right now is with RawFdContainer. rustix doesn't deal with RawFds, it uses I/O-safe BorrowedFd and OwnedFd types (not the ones used in Rustc 1.63). This means that I have to reimplement RawFdContainer as a newtype around OwnedFd. This is a breaking change, and I would like to avoid it if possible, but as far as I know there isn't a real way around it.

I'm filing this as a draft for now to discuss if there is a way around this.

psychon

Basically 👍 . I didn't find anything that made me go "ewww" and I am fine with trying this out.

To answer to your original comment:

The only issue right now is with RawFdContainer. rustix doesn't deal with RawFds, it uses I/O-safe BorrowedFd and OwnedFd types (not the ones used in Rustc 1.63). This means that I have to reimplement RawFdContainer as a newtype around OwnedFd. This is a breaking change, and I would like to avoid it if possible, but as far as I know there isn't a real way around it.

These are actually the fd types from 1.63 if you use that compiler version, I think? Rustix uses io_lifetimes and that auto-detects std support: https://github.com/sunfishcode/io-lifetimes/blob/8aa962eb169deba71b1785076b8f769a41d3d8c8/build.rs#L99-L112

So... this actually starts the conversion away from RawFdContainer to using OwnedFd instead. The two conversions (rustix and io_lifetimes) should IMHO not be entangled. Doing it the way you are starting now means we can use the re-exports from rustix and do not have to explicitly depend on io_lifetimes. ;-)

x11rb-protocol/src/utils.rs

x11rb/Cargo.toml

x11rb/src/rust_connection/stream.rs

psychon · 2023-04-29T05:41:58Z

x11rb/src/rust_connection/stream.rs

+        AddressFamily::UNIX,
+        SocketType::STREAM,
+        SocketFlags::CLOEXEC,
+        Protocol::default(),
    )?;

    // Wrap it in a RawFdContainer. Its Drop impl makes sure to close the socket if something
    // errors out below.


Well... this comment could be updated. The Drop impl of RawFdContainer is no longer relevant for this, since this is already an OwnedFd, right? (In fact, there is no longer a Drop impl)

notgull · 2023-04-30T04:08:21Z

So... this actually starts the conversion away from RawFdContainer to using OwnedFd instead. The two conversions (rustix and io_lifetimes) should IMHO not be entangled. Doing it the way you are starting now means we can use the re-exports from rustix and do not have to explicitly depend on io_lifetimes. ;-)

The thing is, though, that we'd have a public dependency on rustix instead. Not the worst thing in the world, but rustix breaks pretty frequently and it would be rough keeping up with that.

I'd rather wait for a month or so for Debian Bookworm to release with Rust 1.63, and then just use OwnedFd from the standard library.

psychon · 2023-04-30T05:36:44Z

Alternatively, we could have a public dependency on io_lifetimes.

I'm not quite sure whether there is much of a difference between the two options: Since rustix uses io_lifetimes and re-exports that, would we really have a public dependency on rustix? No matter which rustix version we use, other code could just use io_lifetimes and that would work. Or, they could just use OwnedFd from the standard library and that would work on Rust 1.63 and newer (but not on older versions).

psychon · 2023-05-06T12:46:25Z

I took the liberty of pushing a commit fixing most of my review comments and fixing compilation of the shared_memory example. Feel free to throw these commits away and do things differently, if you want.

My only remaining comment is this one: #815 (comment)

Running the shared_memory example suggests that I was right and there is something wrong with buffer sizes. Since I am not sure about the C / kernel API for FD passing, I'd prefer if you took a look at this and fixed it. I suppose you know exactly what to do.

$ cargo run -p x11rb --example shared_memory --features "libc shm"
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/examples/shared_memory`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `48`,
 right: `0`', /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/read_sockaddr.rs:158:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Actually... why does this fail with something sockaddr-related? That's not hte error I was expecting. Relevant part of the backtrace:

   3: core::panicking::assert_failed
             at /usr/src/rustc-1.63.0/library/core/src/panicking.rs:181:5
   4: rustix::backend::net::read_sockaddr::read_sockaddr_os
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/read_sockaddr.rs:158:17
   5: rustix::backend::net::read_sockaddr::maybe_read_sockaddr_os
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/read_sockaddr.rs:120:14
   6: rustix::backend::net::syscalls::recvmsg::{{closure}}::{{closure}}
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/syscalls.rs:284:26
   7: core::result::Result<T,E>::map
             at /usr/src/rustc-1.63.0/library/core/src/result.rs:769:25
   8: rustix::backend::net::syscalls::recvmsg::{{closure}}
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/syscalls.rs:281:9
   9: rustix::backend::net::msghdr::with_recv_msghdr
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/msghdr.rs:49:15
  10: rustix::backend::net::syscalls::recvmsg
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/backend/linux_raw/net/syscalls.rs:257:5
  11: rustix::net::send_recv::msg::recvmsg
             at /home/psychon/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.37.19/src/net/send_recv/msg.rs:568:5
  12: <x11rb::rust_connection::stream::DefaultStream as x11rb::rust_connection::stream::Stream>::read
             at ./x11rb/src/rust_connection/stream.rs:447:23

Edit: strace has no problem with the recvmsg parameters:

recvmsg(4, {msg_name={sa_family=AF_UNIX, sun_path=@"/tmp/.X11-unix/X0"}, msg_namelen=128 => 20, msg_iov=[{iov_base="\1\0\v\0\0\0C\16", iov_len=8}], msg_iovlen=1, msg_controllen=0, msg_flags=MSG_CMSG_CLOEXEC}, MSG_CMSG_CLOEXEC) = 8

Edit: Opened bytecodealliance/rustix#660

notgull · 2023-05-06T15:15:24Z

Thanks for pushing! rustix's socket code still might have a few holes in it... probably causes this, yeah

notgull · 2023-05-27T13:48:28Z

It looks like the maintainer of gethostname would like to avoid switching to rustix. Not a problem for now, but to get libc out of our dependency graph it would be nice to either fork it or find something else.

notgull · 2023-07-09T01:59:58Z

Realized that I forgot to mark this as ready when 1.63 dropped in Debian Stable. Oh well. It's about ready now, just needs some straightening out.

eduardosm · 2023-07-15T18:10:54Z

Realized that I forgot to mark this as ready when 1.63 dropped in Debian Stable. Oh well. It's about ready now, just needs some straightening out.

Did I miss something? This PR does not bump the MSRV to 1.63 and uses rustix::fd::OwnedFd.

notgull · 2023-07-15T20:50:55Z

I was waiting until Debian Stable released with v1.63.0 in order to make sure DS users aren't left behind.

psychon · 2023-07-16T09:20:40Z

This adds a dependency on rustix 0.37. rustix bumped its msrv to Rust 1.63 in version 0.38. So, we would be stuck on Rustix 0.37 until our next MSRV bump.

Rustix' readme says:

This crate currently works on the version of Rust on Debian stable, which is currently Rust 1.63. This policy may change in the future, in minor version releases, so users using a fixed version of Rust should pin to a specific version of this crate.

Uhm... we are a lib, we can't really pin this. This makes me feel uneasy since we already had enough msrv-related problems in the past.

On the other hand, x11rb's de-facto MSRV policy is "never update, unless a dependency forces us". Perhaps something like "Debian stable" would actually be a better policy. Dunno.

codecov · 2023-07-17T02:17:22Z

Codecov Report

Patch coverage: 3.57% and project coverage change: +0.04% 🎉

Comparison is base (74ae136) 12.73% compared to head (343be5e) 12.78%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
+ Coverage   12.73%   12.78%   +0.04%     
==========================================
  Files         189      189              
  Lines      137972   138331     +359     
==========================================
+ Hits        17568    17679     +111     
- Misses     120404   120652     +248

Flag	Coverage Δ
tests	`12.78% <3.57%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
x11rb-async/examples/shared_memory_async.rs	`0.00% <ø> (ø)`
x11rb-async/src/rust_connection/extensions.rs	`1.33% <0.00%> (+0.18%)`	⬆️
x11rb-protocol/src/protocol/dri3.rs	`0.00% <0.00%> (ø)`
x11rb-protocol/src/protocol/randr.rs	`0.43% <0.00%> (ø)`
x11rb-protocol/src/protocol/shm.rs	`0.00% <0.00%> (ø)`
x11rb-protocol/src/utils.rs	`100.00% <ø> (+16.10%)`	⬆️
x11rb/src/rust_connection/stream.rs	`4.80% <0.00%> (+0.45%)`	⬆️
x11rb/src/xcb_ffi/mod.rs	`22.22% <0.00%> (-1.42%)`	⬇️
generator/src/generator/namespace/mod.rs	`98.26% <100.00%> (+<0.01%)`	⬆️
x11rb-async/tests/connection_regression_tests.rs	`83.33% <100.00%> (ø)`

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

notgull · 2023-07-17T02:17:23Z

I've updated to rustix 0.38. This involves an MSRV bump to 1.63. I've also completely replaced RawFdContainer with OwnedFd.

Signed-off-by: Uli Schlachter <[email protected]>

notgull · 2023-07-23T15:15:14Z

I think I've addressed the rest of the review comments.

On the other hand, x11rb's de-facto MSRV policy is "never update, unless a dependency forces us". Perhaps something like "Debian stable" would actually be a better policy. Dunno.

smol-rs's MSRV policy is basically this and it's worked well for us. I would support it.

eduardosm · 2023-07-28T17:22:17Z

LGTM

@psychon We'll wait for your final review before merging.

psychon

I am not too happy about the MSRV bump, but I guess I am being too conservative here, so whatever. Basically 👍 except for some nits.

(Do I get the steak dinner also if I write a program to run into this error on purpose, instead of just stumbling upon it accidentally? ;-) )

@eduardosm

@psychon We'll wait for your final review before merging.

Heh, feel free to approve things if you think it is fine. I'm trying not to be a BDFL. ;-)

x11rb/src/rust_connection/stream.rs

.github/workflows/CI.yml

generator/src/generator/namespace/mod.rs

x11rb-protocol/src/utils.rs

x11rb-protocol/Cargo.toml

x11rb/src/rust_connection/stream.rs

Rust 1.63 brings I/O safety, aka std::os::unix::io::OwnedFd. This is quite helpful for FD passing since it allows to get rid of our own RawFdContainer, improving interoperability with other things. This change is already done now in preparation for #815. Let's see what this unleashes in terms of new clippy warnings. Signed-off-by: Uli Schlachter <[email protected]>

Turns out the 1.63 resolver can handle the lifetimes in the async extension functions.

- Update documentation to say 1.63 instead of 1.56 - Remove manual FD count check for sendmsg - Don't derive Hash, PartialEq and Eq for the fallback RawFdContainer - Write a better/unified doc comment for RawFdContainer - Talk about OwnedFd instead of RawFdContainer in namespace generation

psychon

Thanks! And nice solution for de-duplicating the doc comment on the former RawFdContainer.

psychon reviewed Apr 29, 2023

View reviewed changes

psychon mentioned this pull request May 6, 2023

Panic in recvmsg msghdr decoding / read_sockaddr_os() with abstract unix socket address bytecodealliance/rustix#660

Closed

notgull marked this pull request as ready for review July 9, 2023 01:59

notgull force-pushed the notgull/rustix branch from 6eb503f to 6a46fbd Compare July 11, 2023 01:23

notgull and others added 6 commits July 23, 2023 07:56

Replace nix with rustix

fff4169

Address my own review comments for this PR

cda96fd

Signed-off-by: Uli Schlachter <[email protected]>

Fix compilation of shared_memory example

9323801

Signed-off-by: Uli Schlachter <[email protected]>

Fix shm issue

f5e0306

Update to rustix 0.38

a20217f

Add stack CMSG space to Stream recvmsg

663a896

notgull force-pushed the notgull/rustix branch from 381a9ff to 663a896 Compare July 23, 2023 15:07

notgull requested a review from psychon July 23, 2023 15:15

psychon requested changes Jul 29, 2023

View reviewed changes

notgull added 2 commits July 29, 2023 11:40

Remove the old workaround for the 1.56 resolver

2d0bf32

Turns out the 1.63 resolver can handle the lifetimes in the async extension functions.

psychon approved these changes Jul 30, 2023

View reviewed changes

mergify bot merged commit 16a3209 into master Jul 30, 2023

mergify bot deleted the notgull/rustix branch July 30, 2023 06:23

This was referenced Jul 30, 2023

Should we bump the MSRV for OwnedFd? #750

Closed

Make use of Rust 1.63 features #863

Merged

daxpedda mentioned this pull request Oct 5, 2023

Update x11rb to v0.12 1Password/arboard#101

Merged

patowen mentioned this pull request Nov 18, 2023

Update winit to 0.29.3 Ralith/hypermine#325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace nix with rustix #815

Replace nix with rustix #815

notgull commented Apr 28, 2023

psychon left a comment

psychon Apr 29, 2023

notgull commented Apr 30, 2023

psychon commented Apr 30, 2023

psychon commented May 6, 2023 •

edited

Loading

notgull commented May 6, 2023

notgull commented May 27, 2023

notgull commented Jul 9, 2023

eduardosm commented Jul 15, 2023

notgull commented Jul 15, 2023

psychon commented Jul 16, 2023

codecov bot commented Jul 17, 2023 •

edited

Loading

notgull commented Jul 17, 2023

notgull commented Jul 23, 2023

eduardosm commented Jul 28, 2023

psychon left a comment

psychon left a comment

Replace nix with rustix #815

Replace nix with rustix #815

Conversation

notgull commented Apr 28, 2023

psychon left a comment

Choose a reason for hiding this comment

psychon Apr 29, 2023

Choose a reason for hiding this comment

notgull commented Apr 30, 2023

psychon commented Apr 30, 2023

psychon commented May 6, 2023 • edited Loading

notgull commented May 6, 2023

notgull commented May 27, 2023

notgull commented Jul 9, 2023

eduardosm commented Jul 15, 2023

notgull commented Jul 15, 2023

psychon commented Jul 16, 2023

codecov bot commented Jul 17, 2023 • edited Loading

Codecov Report

notgull commented Jul 17, 2023

notgull commented Jul 23, 2023

eduardosm commented Jul 28, 2023

psychon left a comment

Choose a reason for hiding this comment

psychon left a comment

Choose a reason for hiding this comment

psychon commented May 6, 2023 •

edited

Loading

codecov bot commented Jul 17, 2023 •

edited

Loading