Skip to content

mman module netbsd additions. #1520

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
Merged
merged 1 commit into from
Sep 12, 2021
Merged

mman module netbsd additions. #1520

merged 1 commit into from
Sep 12, 2021

Conversation

devnexen
Copy link
Contributor

No description provided.

asomers
asomers requested changes Sep 12, 2021
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.

Could you please add a test? Probably you can just enable the existing test_mremap_grow and test_mremap_shrink for NetBSD?

src/sys/mman.rs Outdated
@@ -351,6 +363,29 @@ pub unsafe fn mremap(
}
}

#[cfg(target_os = "netbsd")]
Copy link
Member

Choose a reason for hiding this comment

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

This function already exists on Linux, and it looks to me like the only different is the operand order. In the interest of portability, could you please share a single signature for the two operating systems? You can use cfg_if! around the libc syscall to handle the different operand orders.

src/sys/mman.rs Outdated
@@ -150,6 +152,16 @@ libc_bitflags!{
}
}

#[cfg(target_os = "netbsd")]
Copy link
Member

Choose a reason for hiding this comment

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

You should share this definition with the Linux one.

src/sys/mman.rs Outdated
@@ -136,6 +136,8 @@ libc_bitflags!{
MAP_NOCACHE;
#[cfg(any(target_os = "ios", target_os = "macos"))]
MAP_JIT;
#[cfg(target_os = "freebsd")]
MAP_ALIGNED_SUPER;
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated to the mremap stuff. Better to put it in its own PR.

@devnexen devnexen force-pushed the mman_bsd_upd branch 2 times, most recently from b9c4167 to a477541 Compare September 12, 2021 15:39
@devnexen devnexen changed the title mman module freebsd and netbsd additions. mman module netbsd additions. Sep 12, 2021
asomers
asomers approved these changes Sep 12, 2021
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.

LGTM, assuming you ran the tests on NetBSD. CI doesn't.

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 12, 2021

Build succeeded:

@bors bors bot merged commit 1b7e485 into nix-rust:master Sep 12, 2021
@rtzoeller
Copy link
Collaborator

@asomers while testing #1624 I found that test_mremap_shrink was failing with ENOMEM on NetBSD. After investigating it seems like this test was never actually run on NetBSD when this PR was authored. 😞 The documentation added for MAP_FIXED is also incorrect, and the confusion seems to be why the test is failing.

I'll submit a PR (another day) to clean all this up.

The NetBSD man page for mremap states

     MAP_FIXED          newp is tried and mremap() fails if that address can't
                        be used as new base address for the range.  Otherwise,
                        oldp and newp are used as hints for the position,
                        factoring in the given alignment.

Whereas nix's documentation for the corresponding parameter new_address states

Permits to use the old and new address as hints to relocate the mapping.

I believe nix's documentation is completely backwards. If MAP_FIXED is not specified, newp is just a hint. But if MAP_FIXED is specified, NetBSD attempts to use newp and fails if it can't.

The test passes None for newp/new_address, which is converted to NULL which will never work.

On a related note, I have a NetBSD VM now.

@devnexen
Copy link
Contributor Author

devnexen commented Jan 2, 2022

@asomers while testing #1624 I found that test_mremap_shrink was failing with ENOMEM on NetBSD.

What happens if you do ulimit -c unlimited ?

@rtzoeller
Copy link
Collaborator

@devnexen no change, test still fails.

@devnexen
Copy link
Contributor Author

devnexen commented Jan 2, 2022

in my netbsd laptop 3 times out of 4 it passes however if indeed I put a address in the last argument it always pass.

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.
3 participants
@devnexen @rtzoeller @asomers