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

Port memfd to rustix. #27

Merged
merged 3 commits into from
Feb 28, 2022
Merged

Port memfd to rustix. #27

merged 3 commits into from
Feb 28, 2022

Conversation

sunfishcode
Copy link
Contributor

This ports memfd from using the libc crate directly to using rustix, a syscall
wrapper crate with a focus on safe APIs. This factors out several unsafe
blocks, C string conversions, and error handling blocks.

Rustix's MSRV is 1.48, so this would effectively bump memfd's MSRV to 1.48;
Please feel free to decline this PR if you don't wish to take on these new
requirements.

This ports memfd from using the libc crate directly to using [rustix], a syscall
wrapper crate with a focus on safe APIs. This factors out several unsafe
blocks, C string conversions, and error handling blocks.

Rustix's MSRV is 1.48, so this would effectively bump memfd's MSRV to 1.48;
Please feel free to decline this PR if you don't wish to take on these new
requirements.

[rustix]: https://crates.io/crates/rustix
@nagisa
Copy link
Collaborator

nagisa commented Jan 27, 2022

This seems pretty nice to me at a quick scan. I think the most likely reason to not do this will be related to build times, if there's anything at all.

@sunfishcode
Copy link
Contributor Author

Ah, it does add about 2 seconds to the build, on my machine; is that too much? In any case, I'll investigate the times to see if it can go faster.

tests/memfd.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@lucab
Copy link
Owner

lucab commented Jan 27, 2022

Thanks for the PR! Big fan of cap-std and rustix here, if @nagisa likes it too I'm happy to take it (I think they were not super happy with having nix, in the past).
The MSRV bump seems fine to me, the new lower bound looks reasonable.

To be honest I have one big concern about exposing rustix types in the API here. That crate is still semver-moving fast (for good reasons), and on other projects I work on we already got trapped into tangled updates of multiple crates consuming different versions of rustix/cap-*/io-lifetimes/etc (see https://github.com/cgwalters/cap-std-ext/pull/1 for one example).

To that extent, I'd feel much relieved if we could stick to suboptimal-but-boring stdlib/libc types in the signatures (e.g. in errors, constants, etc) and only use rustix internally, for the moment.

@sunfishcode
Copy link
Contributor Author

To that extent, I'd feel much relieved if we could stick to suboptimal-but-boring stdlib/libc types in the signatures (e.g. in errors, constants, etc) and only use rustix internally, for the moment.

Makes sense. I've now fixed the use of rustix::io::Error in the public API, which I believe was the only place it was exposed.

@nagisa
Copy link
Collaborator

nagisa commented Jan 30, 2022

I think they were not super happy with having nix, in the past

My issue with the nix crate in the past was that it has soundness issues or some such at the time and if I recall correctly they were not being addressed; I don't recall exact details, though.


Ah, it does add about 2 seconds to the build, on my machine; is that too much?

Two seconds seem reasonable enough.

That said, one other thought I had just now in favour of libc over rustix – libc is 0.2 and will remain 0.2 for the foreseeable future. This has a couple benefits, primary of which is a lower maintenance burden. Crates depending on libc are effectively always up-to-date (well, so long as they use a ^ dependency bound) and memfd won't be a cause of dependency duplication in downstream crates.

I feel like if we wanted to adopt something like rustix, setting up a dependabot or somesuch would be a good idea. Using >= ... && < ... dependency bounds sounds like a potentially not an outlandish idea as well.

That said… (in favour of rustix based implementation), I also consider an ability to build memfd-rs without any dependency on an external libc library to be pretty enticing as well.

Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Purely from the perspective of implementation this seems sound.

@@ -4,8 +4,6 @@ use std::fmt;
/// Enumeration of errors possible in this library
#[derive(Debug)]
pub enum Error {
/// Cannot convert the `name` argument to a C String!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self when releasing: this is a breaking change, so a semver-breaking version bump is necessary.

@sunfishcode
Copy link
Contributor Author

I investigated the build times, and found a few things that were easy to speed up. Rustix 0.33.1 now includes some build speedups which takes the extra build time down to about 1.5 seconds.

I believe I've addressed all the review feedback now.

@nagisa
Copy link
Collaborator

nagisa commented Feb 27, 2022

NB: I still haven't forgotten this and have it in my TODO list. I'm hoping to one day get some time to set up dependabot or similar alert for dependency bounds.

@sunfishcode are you looking to use memfd for anything specific that would benefit from this being merged sooner rather than later, or is it fine for this change to wait for whenever I've an opportunity to prepare the infra for maintaining this impl?

@lucab
Copy link
Owner

lucab commented Feb 28, 2022

I did open #28 with basic dependabot config.

PR looks good to me. I'd say we can merge this now, and do further tweaks in followups.
@nagisa feel free to merge whenever it feels ok for you.

@nagisa nagisa merged commit d77f575 into lucab:master Feb 28, 2022
@sunfishcode
Copy link
Contributor Author

Thanks! I see there's a build failure on Windows here. I'm preparing bytecodealliance/rustix#242 with a fix.

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