Overhaul the API #42

bugaevc · May 10, 2017

Overhaul the API as requested in #33. In particular,

Remove MmapView and MmapViewSync types
Define two types - Mmap and MmapMut for readable and readable/writable mappings respectively. They implement Deref<Target = [u8]>/DerefMut as appropriate.
- Mmap has no flush methods.
- Alongside "raw" set_protection(), provide make_mut() and make_read_only() to safely convert between Mmap and MmapMut.
Get rid of multiple open, open_path, anonymous_with_options, ... methods. Instead, provide two OpenOptions-style builders, one for file-backed and one for anonymous mappings.
- Remove support for opening files by their path. Always use std::fs::File.
- On Unix, memmap::file() is unsafe. This seems to be the best approach, although it is always safe to use it with ReadCopy.
- To improve ergonomics, automatically pick a suitable Protection unless specified explicitly.
  - This makes it ergonomic enough so there's no need for additional convenience constructors.
Convert the tests and the cat example to use the new API.
- use super as memmap doesn't seem to work, rustc says that super is not found because it's the root module, WTF? Yet, use super::* works. Check out the workaround I came up with.

bugaevc · May 10, 2017

The build is failing on Windows because of unnecessary unsafes (should I just #[allow] them?) and on Rust 1.8 because I used newer features such as the ? operator.

jonas-schievink · May 10, 2017

What happens if I create a MmapMut and call set_protection(Protection::Read)? Will a write just segfault?

bugaevc · May 10, 2017

What happens if I create a MmapMut and call set_protection(Protection::Read)? Will a write just segfault?

No, set_protection() will return Err appropriately.

An alternative approach would be to split Protection into separate enums for mutable and immutable options. That would allow for things like this to be enforced statically, but it seems to make APIs less ergonomic.

jonas-schievink · May 10, 2017

Ah, no, that's fine. I didn't see it in the diff because it wasn't getting rendered.

danburkert · May 11, 2017

danburkert requested changes May 11, 2017

View changes

Hey @bugaevc, thanks for putting in the time to do this. I left some detailed nits, but I want to reiterate on a couple of higher level points:

I'd prefer if we kept the normal user facing API completely the same for Windows and Unix, even if that means overshooting with marking things unsafe on Windows. Having a consistent cross-platform API has always been the goal of this crate. We are planning on introducing platform-specific extension traits in the future, but that will be for exposing functionality that fundamentally isn't supported across platforms.
I think this would be easier to review if you split out the removal of the view APIs into a separate commit.
We probably need to think about what to do about Protections. I think it's pretty clear that they aren't the right abstraction given the split in Mmap and MmapMut. Anyone have thoughts for a better way forward?

CC #33, @sfackler, @BurntSushi, I'd appreciate you alls thoughts if you have any

danburkert · May 11, 2017

src/lib.rs

+/// then chain call methods to configure additional options, finally, call [`map()`](#method.map)
+/// or [`map_mut()`](#method.map_mut).
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
+pub struct AnonymousMappingOptions {


I think AnonymousMmapOptions is a better name for this, so it lines up with the type it's building.

danburkert · May 11, 2017

src/lib.rs

+
+    /// Actually map this anonymous mapping into the address space.
+    ///
+    /// This methos returns an immutable mapping, see [`map_mut()`](#method.map_mut)


typo: method

danburkert · May 11, 2017

src/lib.rs

+    ///
+    /// This method returns `Err` when the underlying system call fails, which can happen for
+    /// a variety of reasons, such as when you don't have the necessary permissions for the file.
+    pub fn map(&self) -> Result<Mmap> {


Does this constructor actually make sense? What's the usecase for creating a read-only anonymous memory map?

Yeah, I thought about this too, but decided to leave it here for consistency. I should however document that this is not what you want most of the time.

I think we should go ahead and remove it unless there's a compelling reason to keep it at this point. We can always add it back later if a case can be made.

danburkert · May 11, 2017

src/lib.rs

+/// [`Protection::ReadCopy`](enum.Protection.html#variant.ReadCopy)
+/// is always safe.
+#[cfg(unix)]
+pub unsafe fn file(file: &File) -> FileMappingOptions {


I would strongly prefer if this API were the same across all platforms, even if that means marking it unsafe on Windows.

danburkert · May 11, 2017

src/lib.rs

+/// In particular, it is **undefined behavior** in Rust for the memory to be
+/// modified by some other code while there's a reference to it.
+///
+/// Note, however, that using this with


This has never really been clear to me. The Linux mmap manpage says:

It is unspecified whether changes made to the file after the mmap() call are visible in the mapped region.

That doesn't seem like it will play well with Rust's safety semantics.

Ah, right. I'll change the message to say it's unsafe even with cow.

danburkert · May 11, 2017

src/lib.rs

+    ///
+    /// This method *also* returns `Err` with `ErrorKind` set to `InvalidInput` if the specified
+    /// protection does not allow the mapping to be mutable.
+    pub fn set_protection(&mut self, protection: Protection) -> Result<()> {


So this is just for switching from ReadWrite to ReadCopy and vice versa? That seems kinda strange - we should probably document what that even means. I'm surprised the OS allows it.

danburkert · May 11, 2017

src/lib.rs

-            len: self.len,
-        }
+    /// This method will **not** return `Err` if the passed `protection` is mutable.
+    pub fn make_read_only(mut self, protection: Protection) -> Result<Mmap> {


Perhaps this could be make_read_only, and make_read_execute? Are there any other valid outcomes from this?

danburkert · May 11, 2017

src/lib.rs

+    #![allow(unused_unsafe)]
+
+    mod memmap {
+        pub use super::super::*;


Is it not possible to do a use super::*; here without the extra mod? I've never seen anything like this.

It is, I just wanted the tests code to say memmap::file instead of just file, etc. As I said above, use super as memmap doesn't work, this is either a bug or me misunderstanding how the module system works.

danburkert · May 11, 2017

src/windows.rs

@@ -78,13 +77,7 @@ impl MmapInner {
        }
    }

-    /// Check if a file needs to be writable for copy-on-write mode.
-    #[inline]
-    pub fn needs_write_for_copy() -> bool {


Bad rebase?

No, actually, this method isn't used anymore, because we require for the file to be opened in advance.

danburkert · May 11, 2017

src/lib.rs

+                ErrorKind::InvalidInput,
+                "Invalid protection for a mutable mapping",
+            )),
+            Protection::ReadWrite | Protection::ReadCopy => Ok(


Does ReadCopy even make sense for an Anonymous mapping? I'm having a hard time reasoning through some of these semantics.

No, not really; again, it's here for consistency. Do you think we should work out what combinations are actually useful and only allow those?

Yah, I think eventually we should, but we can punt for now and do it in a follow-up commit.

bugaevc · May 11, 2017

We could get rid of protection in public API entirely, instead exposing a bunch of methods: {map, make}_read_{only, write, execute, copy_on_write} for initial mapping and conversions where they make sense.

bugaevc · May 11, 2017

Rebased to separate MmapView removal and addressed a couple of pointed out issues: naming (Mapping -> Mmap in Options), a typo ("methos") and API difference between Unix and Windows (it's now always unsafe to mmap a file).

Please let me know what you think about my suggestion above.

danburkert · May 12, 2017

danburkert requested changes May 12, 2017

View changes

Looks good, just a few comments. Could you switch the ? calls to try! so CI can pass?

danburkert · May 12, 2017

src/lib.rs

+    ///
+    /// This method returns `Err` when the underlying system call fails, which can happen for
+    /// a variety of reasons, such as when you don't have the necessary permissions for the file.
+    pub fn map(&self) -> Result<Mmap> {


I think we should go ahead and remove it unless there's a compelling reason to keep it at this point. We can always add it back later if a case can be made.

danburkert · May 12, 2017

src/lib.rs

+                ErrorKind::InvalidInput,
+                "Invalid protection for a mutable mapping",
+            )),
+            Protection::ReadWrite | Protection::ReadCopy => Ok(


Yah, I think eventually we should, but we can punt for now and do it in a follow-up commit.

danburkert · May 12, 2017

src/windows.rs

@@ -78,13 +77,7 @@ impl MmapInner {
        }
    }

-    /// Check if a file needs to be writable for copy-on-write mode.
-    #[inline]
-    pub fn needs_write_for_copy() -> bool {


danburkert · May 12, 2017

I merged the view removal commit, so you may need to rebase over that.

bugaevc · May 12, 2017

Removed AnonymousMmapOptions::map() and the corresponding test.
Made it compile under 1.8.0 (why do you need that?)

danburkert · May 12, 2017

Awesome, thanks. As for Rust 1.8, there is no hard and fast requirement except that that's the status quo, so if it's not too onerous to maintain I'd prefer to. This crate is likely to be a widely used dependency, and if it forces a certain rustic version, then it's forced on everyone who uses it as well. Basically the same reason that libc maintains long backwards compat windows.

Will take a look and likely merge tonight or over the weekend. Thanks again for the help!

danburkert · May 12, 2017

danburkert approved these changes May 12, 2017

View changes

bugaevc · May 12, 2017

Great! So, if you think this:

We could get rid of protection in public API entirely, instead exposing a bunch of methods: {map, make}_read_{only, write, execute, copy_on_write} for initial mapping and conversions where they make sense.

is worth a try! 😄 , I'll make a separate PR, is that OK?

bugaevc · May 12, 2017

Also note that I haven't put much work into docs here, that's left for #32 and #34

bugaevc · May 20, 2017

Will take a look and likely merge tonight or over the weekend.

Any news?

danburkert · May 22, 2017

Thanks!

bugaevc force-pushed the bugaevc:overhaul-api branch 2 times, most recently from 1a768dc to 6384afc May 11, 2017

bugaevc added some commits May 10, 2017

Overhaul the API

91e2ebc

Downgrade code to compile under Rust 1.8.0

Loading status checks…

3ffc31b

bugaevc force-pushed the bugaevc:overhaul-api branch from 6384afc to 3ffc31b May 12, 2017

danburkert merged commit da7000b into danburkert:master May 22, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

danburkert/memmap-rs

Join GitHub today

Overhaul the API #42

Conversation

bugaevc commented May 10, 2017

This comment has been minimized.

bugaevc commented May 10, 2017

This comment has been minimized.

jonas-schievink commented May 10, 2017

This comment has been minimized.

bugaevc commented May 10, 2017

This comment has been minimized.

jonas-schievink commented May 10, 2017

danburkert requested changes May 11, 2017 View changes

danburkert left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bugaevc commented May 11, 2017 • edited

bugaevc force-pushed the bugaevc:overhaul-api branch 2 times, most recently from 1a768dc to 6384afc May 11, 2017

This comment has been minimized.

bugaevc commented May 11, 2017

danburkert requested changes May 12, 2017 View changes

danburkert left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

danburkert commented May 12, 2017

bugaevc added some commits May 10, 2017

bugaevc force-pushed the bugaevc:overhaul-api branch from 6384afc to 3ffc31b May 12, 2017

This comment has been minimized.

bugaevc commented May 12, 2017

This comment has been minimized.

danburkert commented May 12, 2017

danburkert approved these changes May 12, 2017 View changes

This comment has been minimized.

bugaevc commented May 12, 2017

This comment has been minimized.

bugaevc commented May 12, 2017

This comment has been minimized.

bugaevc commented May 20, 2017

Hide details View details danburkert merged commit da7000b into danburkert:master May 22, 2017 2 checks passed

2 checks passed

This comment has been minimized.

danburkert commented May 22, 2017

danburkert requested changes May 11, 2017

View changes

danburkert left a comment •

edited

bugaevc commented May 11, 2017 •

edited

bugaevc force-pushed the bugaevc:overhaul-api branch 2 times, most recently from `1a768dc` to `6384afc` May 11, 2017

danburkert requested changes May 12, 2017

View changes

bugaevc force-pushed the bugaevc:overhaul-api branch from `6384afc` to `3ffc31b` May 12, 2017

danburkert approved these changes May 12, 2017

View changes

danburkert merged commit `da7000b` into danburkert:master May 22, 2017
2 checks passed