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

Add wrapper for mremap #1306

Merged
merged 1 commit into from Oct 13, 2020
Merged

Add wrapper for mremap #1306

merged 1 commit into from Oct 13, 2020

Conversation

sporksmith
Copy link
Contributor

@sporksmith sporksmith commented Oct 5, 2020

Fixes #1295

@sporksmith sporksmith marked this pull request as ready for review Oct 5, 2020
@sporksmith
Copy link
Contributor Author

sporksmith commented Oct 5, 2020

It looks like the libc crate doesn't have mremap in the FreeBSD CI environment:

error[E0425]: cannot find function `mremap` in module `libc`
   --> src/sys/mman.rs:330:21
    |
330 |     let ret = libc::mremap(addr, old_size, new_size, flags.bits(), new_address);
    |                     ^^^^^^
help: a function with a similar name exists
    |
330 |     let ret = libc::mmap(addr, old_size, new_size, flags.bits(), new_address);
    |                     ^^^^
help: possible candidate is found in another module, you can import it into scope
    |
1   | use crate::sys::mman::mremap;
    |

I assume I need to config-guard this wrapper, but I'm not sure how to tell which configs ought to include it. I don't see any such config-guard in the libc crate itself: https://docs.rs/libc/0.2.68/src/libc/unix/linux_like/linux/mod.rs.html#2910

Any ideas?

@sporksmith
Copy link
Contributor Author

sporksmith commented Oct 5, 2020

I assume I need to config-guard this wrapper, but I'm not sure how to tell which configs ought to include it. I don't see any such config-guard in the libc crate itself: https://docs.rs/libc/0.2.68/src/libc/unix/linux_like/linux/mod.rs.html#2910

Added a guard to make it Linux-only, since the man page notes that it's Linux-specific.

@sporksmith
Copy link
Contributor Author

sporksmith commented Oct 6, 2020

Hmm, I'm fairly certain the CI failure is spurious. It doesn't look related to this PR, and it previously passed on 7a46b52, which should be exactly the same.

I'm happy to rerun if desired; is there a nicer way to force a rerun than pushing an empty commit?

src/sys/mman.rs Outdated
old_size: size_t,
new_size: size_t,
flags: MapFlags,
new_address: size_t,
Copy link
Member

@asomers asomers Oct 10, 2020

Choose a reason for hiding this comment

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

Shouldn't new_address be optional?

Copy link
Contributor Author

@sporksmith sporksmith Oct 11, 2020

Choose a reason for hiding this comment

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

Yes; I wasn't sure the best way to handle this since Rust doesn't support variadic functions. The argument is ignored if MAP_FIXED isn't in flags; i.e. if MAP_FIXED isn't set the caller can put anything there, and it'll be ignored.

Maybe it'd be a bit clearer to make it an Option<size_t>?

Copy link
Contributor Author

@sporksmith sporksmith Oct 11, 2020

Choose a reason for hiding this comment

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

I just realized the flags enum is wrong; will fix that and change new_address to be an Option<size_t> (but lmk if there's another solution you recommend)

Copy link
Contributor Author

@sporksmith sporksmith Oct 11, 2020

Choose a reason for hiding this comment

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

Done. Also added a few tests, though in hindsight maybe these should just be doc tests (and drop the second mremap test)?

@sporksmith sporksmith requested a review from asomers Oct 11, 2020
Copy link
Member

@asomers asomers left a comment

Nice tests.

src/sys/mman.rs Outdated
///
/// # Safety
///
/// See the `mremap(2)` man page for detailed requirements.
Copy link
Member

@asomers asomers Oct 12, 2020

Choose a reason for hiding this comment

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

How about a URL to a man page?

Copy link
Contributor Author

@sporksmith sporksmith Oct 13, 2020

Choose a reason for hiding this comment

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

Done

@sporksmith sporksmith requested a review from asomers Oct 13, 2020
@asomers
Copy link
Member

asomers commented Oct 13, 2020

Looks good. Now it just needs a squash.

@sporksmith
Copy link
Contributor Author

sporksmith commented Oct 13, 2020

Looks good. Now it just needs a squash.

Done

Copy link
Contributor

@asomers-ax asomers-ax left a comment

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 13, 2020

🔒 Permission denied

Existing reviewers: click here to make asomers-ax a reviewer

Copy link
Member

@asomers asomers left a comment

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 13, 2020

Build succeeded:

@bors bors bot merged commit 477743d into nix-rust:master Oct 13, 2020
3 checks passed
asomers added a commit to asomers/nix that referenced this issue Sep 19, 2021
asomers added a commit to asomers/nix that referenced this issue Sep 19, 2021
asomers added a commit to asomers/nix that referenced this issue Sep 19, 2021
asomers added a commit to asomers/nix that referenced this issue Sep 19, 2021
bors bot added a commit that referenced this issue Sep 19, 2021
1534: Actually connect the mman tests to the build r=asomers a=asomers

This was an oversight from #1306.

Reported-by:	`@ocadaruma`

Co-authored-by: Alan Somers <asomers@gmail.com>
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