implement builder for MmapRegion

Currently the MmapRegion provides several methods to create MmapRegion
objects, but we often need to change the existing methods to support
new functionalities. So it would be better to adopt Builder pattern
and implement MmapRegionBuilder, so we could add new functionality
without affecting existing users.

Signed-off-by: Liu Jiang <[email protected]>
Signed-off-by: Alexandru Agache <[email protected]>

Author: jiangliu

CHANGELOG.md

@@ -3,6 +3,7 @@
 
 ### Added
 
+  - [[#149]](https://github.com/rust-vmm/vm-memory/issues/149): Implement builder for MmapRegion.
   - [[#140]](https://github.com/rust-vmm/vm-memory/issues/140): Add dirty bitmap tracking abstractions. 
 
 ### Deprecated 

coverage_config_x86_64.json

@@ -1,5 +1,5 @@
 {
-  "coverage_score": 88.1,
+  "coverage_score": 88.5,
   "exclude_path": "mmap_windows.rs",
   "crate_features": "backend-mmap,backend-atomic,backend-bitmap"
 }

src/mmap.rs

@@ -31,7 +31,7 @@ use crate::volatile_memory::{VolatileMemory, VolatileSlice};
 use crate::{AtomicAccess, Bytes};
 
 #[cfg(unix)]
-pub use crate::mmap_unix::{Error as MmapRegionError, MmapRegion};
+pub use crate::mmap_unix::{Error as MmapRegionError, MmapRegion, MmapRegionBuilder};
 
 #[cfg(windows)]
 pub use crate::mmap_windows::MmapRegion;

src/mmap_unix.rs

@@ -74,6 +74,154 @@ impl error::Error for Error {}
 
 pub type Result<T> = result::Result<T, Error>;
 
+/// A factory struct to build `MmapRegion` objects.
+pub struct MmapRegionBuilder<B = ()> {
+    size: usize,
+    prot: i32,
+    flags: i32,
+    file_offset: Option<FileOffset>,
+    raw_ptr: Option<*mut u8>,
+    hugetlbfs: Option<bool>,
+    bitmap: B,
+}
+
+impl<B: Bitmap + Default> MmapRegionBuilder<B> {
+    /// Create a new `MmapRegionBuilder` using the default value for
+    /// the inner `Bitmap` object.
+    pub fn new(size: usize) -> Self {
+        Self::new_with_bitmap(size, B::default())
+    }
+}
+
+impl<B: Bitmap> MmapRegionBuilder<B> {
+    /// Create a new `MmapRegionBuilder` using the provided `Bitmap` object.
+    ///
+    /// When instantiating the builder for a region that does not require dirty bitmap
+    /// bitmap tracking functionality, we can specify a trivial `Bitmap` implementation
+    /// such as `()`.
+    pub fn new_with_bitmap(size: usize, bitmap: B) -> Self {
+        MmapRegionBuilder {
+            size,
+            prot: 0,
+            flags: libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
+            file_offset: None,
+            raw_ptr: None,
+            hugetlbfs: None,
+            bitmap,
+        }
+    }
+
+    /// Create the `MmapRegion` object with the specified mmap memory protection flag `prot`.
+    pub fn with_mmap_prot(mut self, prot: i32) -> Self {
+        self.prot = prot;
+        self
+    }
+
+    /// Create the `MmapRegion` object with the specified mmap `flags`.
+    pub fn with_mmap_flags(mut self, flags: i32) -> Self {
+        self.flags = flags;
+        self
+    }
+
+    /// Create the `MmapRegion` object with the specified `file_offset`.
+    pub fn with_file_offset(mut self, file_offset: FileOffset) -> Self {
+        self.file_offset = Some(file_offset);
+        self
+    }
+
+    /// Create the `MmapRegion` object with the specified `hugetlbfs` flag.
+    pub fn with_hugetlbfs(mut self, hugetlbfs: bool) -> Self {
+        self.hugetlbfs = Some(hugetlbfs);
+        self
+    }
+
+    /// Create the `MmapRegion` object with pre-mmapped raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// To use this safely, the caller must guarantee that `raw_addr` and `self.size` define a
+    /// region within a valid mapping that is already present in the process.
+    pub unsafe fn with_raw_mmap_pointer(mut self, raw_ptr: *mut u8) -> Self {
+        self.raw_ptr = Some(raw_ptr);
+        self
+    }
+
+    /// Build the `MmapRegion` object.
+    pub fn build(self) -> Result<MmapRegion<B>> {
+        if self.raw_ptr.is_some() {
+            return self.build_raw();
+        }
+
+        // Forbid MAP_FIXED, as it doesn't make sense in this context, and is pretty dangerous
+        // in general.
+        if self.flags & libc::MAP_FIXED != 0 {
+            return Err(Error::MapFixed);
+        }
+
+        let (fd, offset) = if let Some(ref f_off) = self.file_offset {
+            check_file_offset(f_off, self.size)?;
+            (f_off.file().as_raw_fd(), f_off.start())
+        } else {
+            (-1, 0)
+        };
+
+        // This is safe because we're not allowing MAP_FIXED, and invalid parameters cannot break
+        // Rust safety guarantees (things may change if we're mapping /dev/mem or some wacky file).
+        let addr = unsafe {
+            libc::mmap(
+                null_mut(),
+                self.size,
+                self.prot,
+                self.flags,
+                fd,
+                offset as libc::off_t,
+            )
+        };
+
+        if addr == libc::MAP_FAILED {
+            return Err(Error::Mmap(io::Error::last_os_error()));
+        }
+
+        Ok(MmapRegion {
+            addr: addr as *mut u8,
+            size: self.size,
+            bitmap: self.bitmap,
+            file_offset: self.file_offset,
+            prot: self.prot,
+            flags: self.flags,
+            owned: true,
+            hugetlbfs: self.hugetlbfs,
+        })
+    }
+
+    fn build_raw(self) -> Result<MmapRegion<B>> {
+        // Safe because this call just returns the page size and doesn't have any side effects.
+        let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize;
+        let addr = self.raw_ptr.clone().unwrap();
+
+        // Check that the pointer to the mapping is page-aligned.
+        if (addr as usize) & (page_size - 1) != 0 {
+            return Err(Error::InvalidPointer);
+        }
+
+        // Check that the size is a multiple of the page size.
+        if self.size & (page_size - 1) != 0 {
+            return Err(Error::InvalidSize);
+        }
+
+        Ok(MmapRegion {
+            addr,
+            size: self.size,
+            bitmap: self.bitmap,
+            file_offset: self.file_offset,
+            prot: self.prot,
+            flags: self.flags,
+            owned: false,
+            hugetlbfs: self.hugetlbfs,
+        })
+    }
+}
+
 /// Helper structure for working with mmaped memory regions in Unix.
 ///
 /// The structure is used for accessing the guest's physical memory by mmapping it into
@@ -108,12 +256,10 @@ impl<B: NewBitmap> MmapRegion<B> {
     /// # Arguments
     /// * `size` - The size of the memory region in bytes.
     pub fn new(size: usize) -> Result<Self> {
-        Self::build(
-            None,
-            size,
-            libc::PROT_READ | libc::PROT_WRITE,
-            libc::MAP_ANONYMOUS | libc::MAP_NORESERVE | libc::MAP_PRIVATE,
-        )
+        MmapRegionBuilder::new_with_bitmap(size, B::with_len(size))
+            .with_mmap_prot(libc::PROT_READ | libc::PROT_WRITE)
+            .with_mmap_flags(libc::MAP_ANONYMOUS | libc::MAP_NORESERVE | libc::MAP_PRIVATE)
+            .build()
     }
 
     /// Creates a shared file mapping of `size` bytes.
@@ -123,12 +269,11 @@ impl<B: NewBitmap> MmapRegion<B> {
     ///                   referred to by `file_offset.file`.
     /// * `size` - The size of the memory region in bytes.
     pub fn from_file(file_offset: FileOffset, size: usize) -> Result<Self> {
-        Self::build(
-            Some(file_offset),
-            size,
-            libc::PROT_READ | libc::PROT_WRITE,
-            libc::MAP_NORESERVE | libc::MAP_SHARED,
-        )
+        MmapRegionBuilder::new_with_bitmap(size, B::with_len(size))
+            .with_file_offset(file_offset)
+            .with_mmap_prot(libc::PROT_READ | libc::PROT_WRITE)
+            .with_mmap_flags(libc::MAP_NORESERVE | libc::MAP_SHARED)
+            .build()
     }
 
     /// Creates a mapping based on the provided arguments.
@@ -147,37 +292,13 @@ impl<B: NewBitmap> MmapRegion<B> {
         prot: i32,
         flags: i32,
     ) -> Result<Self> {
-        // Forbid MAP_FIXED, as it doesn't make sense in this context, and is pretty dangerous
-        // in general.
-        if flags & libc::MAP_FIXED != 0 {
-            return Err(Error::MapFixed);
-        }
-
-        let (fd, offset) = if let Some(ref f_off) = file_offset {
-            check_file_offset(f_off, size)?;
-            (f_off.file().as_raw_fd(), f_off.start())
-        } else {
-            (-1, 0)
-        };
-
-        // This is safe because we're not allowing MAP_FIXED, and invalid parameters cannot break
-        // Rust safety guarantees (things may change if we're mapping /dev/mem or some wacky file).
-        let addr = unsafe { libc::mmap(null_mut(), size, prot, flags, fd, offset as libc::off_t) };
-
-        if addr == libc::MAP_FAILED {
-            return Err(Error::Mmap(io::Error::last_os_error()));
+        let mut builder = MmapRegionBuilder::new_with_bitmap(size, B::with_len(size))
+            .with_mmap_prot(prot)
+            .with_mmap_flags(flags);
+        if let Some(v) = file_offset {
+            builder = builder.with_file_offset(v);
         }
-
-        Ok(Self {
-            addr: addr as *mut u8,
-            size,
-            bitmap: B::with_len(size),
-            file_offset,
-            prot,
-            flags,
-            owned: true,
-            hugetlbfs: None,
-        })
+        builder.build()
     }
 
     /// Creates a `MmapRegion` instance for an externally managed mapping.
@@ -198,29 +319,11 @@ impl<B: NewBitmap> MmapRegion<B> {
     /// To use this safely, the caller must guarantee that `addr` and `size` define a region within
     /// a valid mapping that is already present in the process.
     pub unsafe fn build_raw(addr: *mut u8, size: usize, prot: i32, flags: i32) -> Result<Self> {
-        // Safe because this call just returns the page size and doesn't have any side effects.
-        let page_size = libc::sysconf(libc::_SC_PAGESIZE) as usize;
-
-        // Check that the pointer to the mapping is page-aligned.
-        if (addr as usize) & (page_size - 1) != 0 {
-            return Err(Error::InvalidPointer);
-        }
-
-        // Check that the size is a multiple of the page size.
-        if size & (page_size - 1) != 0 {
-            return Err(Error::InvalidSize);
-        }
-
-        Ok(Self {
-            addr,
-            size,
-            bitmap: B::with_len(size),
-            file_offset: None,
-            prot,
-            flags,
-            owned: false,
-            hugetlbfs: None,
-        })
+        MmapRegionBuilder::new_with_bitmap(size, B::with_len(size))
+            .with_raw_mmap_pointer(addr)
+            .with_mmap_prot(prot)
+            .with_mmap_flags(flags)
+            .build()
     }
 }
 
@@ -510,6 +613,17 @@ mod tests {
         assert_eq!(r.prot(), libc::PROT_READ | libc::PROT_WRITE);
         assert_eq!(r.flags(), libc::MAP_NORESERVE | libc::MAP_PRIVATE);
         assert!(r.owned());
+
+        let region_size = 0x10_0000;
+        let bitmap = AtomicBitmap::new(region_size, 0x1000);
+        let builder = MmapRegionBuilder::new_with_bitmap(region_size, bitmap)
+            .with_hugetlbfs(true)
+            .with_mmap_prot(libc::PROT_READ | libc::PROT_WRITE);
+        assert_eq!(builder.size, region_size);
+        assert_eq!(builder.hugetlbfs, Some(true));
+        assert_eq!(builder.prot, libc::PROT_READ | libc::PROT_WRITE);
+
+        crate::bitmap::tests::test_volatile_memory(&(builder.build().unwrap()));
     }
 
     #[test]