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.

Sign up for GitHub

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

Jump to bottom

expose memfd on freebsd #1808

Merged
merged 1 commit into from
Sep 12, 2022
Merged

expose memfd on freebsd #1808

merged 1 commit into from
Sep 12, 2022

Conversation

i509VCB
Copy link
Contributor

@i509VCB i509VCB commented Aug 23, 2022

Resolves #1775

@i509VCB i509VCB force-pushed the freebsd-memfd branch 2 times, most recently from 546512f to 1e93e4c Compare August 23, 2022 23:41
asomers
asomers reviewed Aug 24, 2022
View reviewed changes
src/sys/memfd.rs
@@ -40,7 +42,15 @@ libc_bitflags!(
/// [`memfd_create(2)`]: https://man7.org/linux/man-pages/man2/memfd_create.2.html
pub fn memfd_create(name: &CStr, flags: MemFdCreateFlag) -> Result<RawFd> {
let res = unsafe {
libc::syscall(libc::SYS_memfd_create, name.as_ptr(), flags.bits())
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not bend over backwards to cater to the obscure arches. If they don't define the libc symbol, then I think we should just not bind it. We can do it like this:
#[cfg(any(target_os = "freebsd", target_env = "gnu", target_env = "musl"))]

Copy link
Contributor Author

@i509VCB i509VCB Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry it wasn't architecture, it seems that it's just armv7 uclibceabihf that's failing

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 have some solution but it's a more ugly cfg block.

Essentially use the libc symbol if it's freebsd or gnu/musl linux and otherwise use the syscall function.

Or was the intention to gate memfd_create from all linuxes that aren't using a gnu or musl libc?

@i509VCB i509VCB force-pushed the freebsd-memfd branch 5 times, most recently from a1428e4 to b0531ae Compare August 25, 2022 15:35
@i509VCB i509VCB requested a review from asomers August 30, 2022 02:16
asomers
asomers approved these changes Sep 4, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 4, 2022

Merge conflict.

@i509VCB
Copy link
Contributor Author

i509VCB commented Sep 8, 2022

conflicts are fixed

asomers
asomers approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Sep 8, 2022
@bors @i509VCB
Merge #1808
d92dd40 
1808: expose memfd on freebsd r=asomers a=i509VCB

Resolves #1775

Co-authored-by: i509VCB <git@i509.me>
@bors
Copy link
Contributor

bors bot commented Sep 8, 2022

Build failed:

@i509VCB
Copy link
Contributor Author

i509VCB commented Sep 8, 2022 •
edited
Loading

The failures appear to be unrelated to this pull request. It's new clippy lints firing

@asomers
Copy link
Member

asomers commented Sep 12, 2022

The build should be fixed if you rebase.

@asomers
Copy link
Member

asomers commented Sep 12, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 12, 2022

Build succeeded:

@bors bors bot merged commit 1038ee5 into nix-rust:master Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose memfd on (Free?)BSD
2 participants
@i509VCB @asomers