refactor: scatter mmap.rs contents to unix/windows modules

This is some duplication, but it once and for all eliminates all cfg
fuckery and code that expects precisely one module to be defined.

Admittedly, for windows/unix we could have stuck with the cfg stuff, but
I wanted a clean slate for now. The windows parts are compile-tested
only, as I do not have a windows system available.

The handling of various from_range functions is based on the assumption
that these were only used in test code and nowhere else (hence all the
preceding refactors of test code). In unit tests, their use has been
eliminated, and doc tests now got a cfg(unix. not(feature="xen")) so
that they only run when mmap_unix is available. This is fine, because
they are examples of how to use specific functions, and not any serious
tests (e.g. dont need to run for all supported backends).

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/bitmap/mod.rs

@@ -274,7 +274,7 @@ pub(crate) mod tests {
     // them to test the mmap-based backend implementations for now. Going forward, the generic
     // test functions defined here can be placed in a separate module (i.e. `test_utilities`)
     // which is gated by a feature and can be used for testing purposes by other crates as well.
-    #[cfg(feature = "backend-mmap")]
+    #[cfg(all(feature = "backend-mmap", unix))]
     fn test_guest_memory_region<R: GuestMemoryRegion>(region: &R) {
         let dirty_addr = MemoryRegionAddress(0x0);
         let val = 123u64;
@@ -308,7 +308,7 @@ pub(crate) mod tests {
         );
     }
 
-    #[cfg(feature = "backend-mmap")]
+    #[cfg(all(feature = "backend-mmap", unix))]
     // Assumptions about M generated by f ...
     pub fn test_guest_memory_and_region<M, F>(f: F)
     where

src/bytes.rs

@@ -330,7 +330,7 @@ pub trait Bytes<A> {
     /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(all(feature = "backend-mmap", feature = "rawfd"))]
+    /// # #[cfg(all(feature = "backend-mmap", feature = "rawfd", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap};
     /// # use std::fs::File;

src/guest_memory.rs

@@ -174,7 +174,7 @@ impl FileOffset {
 /// # Examples (uses the `backend-mmap` and `backend-atomic` features)
 ///
 /// ```
-/// # #[cfg(feature = "backend-mmap")]
+/// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
 /// # {
 /// # use std::sync::Arc;
 /// # use vm_memory::{GuestAddress, GuestAddressSpace, GuestMemory, GuestMemoryMmap};
@@ -290,7 +290,7 @@ pub trait GuestMemory {
     ///   `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(feature = "backend-mmap")]
+    /// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{GuestAddress, GuestMemory, GuestMemoryRegion, GuestMemoryMmap};
     /// #
@@ -314,7 +314,7 @@ pub trait GuestMemory {
     /// # Examples (uses the `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(feature = "backend-mmap")]
+    /// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{Address, GuestAddress, GuestMemory, GuestMemoryMmap};
     /// #
@@ -426,7 +426,7 @@ pub trait GuestMemory {
     /// # Examples (uses the `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(feature = "backend-mmap")]
+    /// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{GuestAddress, GuestMemory, GuestMemoryMmap};
     /// #
@@ -483,7 +483,7 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
     /// * Write a slice at guestaddress 0x1000. (uses the `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(feature = "backend-mmap")]
+    /// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{Bytes, GuestAddress, mmap::GuestMemoryMmap};
     /// #
@@ -511,7 +511,7 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
     /// * Read a slice of length 16 at guestaddress 0x1000. (uses the `backend-mmap` feature)
     ///
     /// ```
-    /// # #[cfg(feature = "backend-mmap")]
+    /// # #[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
     /// # {
     /// # use vm_memory::{Bytes, GuestAddress, mmap::GuestMemoryMmap};
     /// #

src/lib.rs

@@ -68,14 +68,22 @@ mod mmap_windows;
 #[cfg(feature = "backend-mmap")]
 pub mod mmap;
 
-#[cfg(feature = "backend-mmap")]
-pub use mmap::{GuestMemoryMmap, GuestRegionMmap, MmapRegion};
 #[cfg(all(feature = "backend-mmap", feature = "xen", unix))]
 pub use mmap::{MmapRange, MmapXenFlags};
 
 #[cfg(all(feature = "xen", unix))]
 pub use mmap_xen::{GuestMemoryXen, MmapRegion as MmapRegionXen};
 
+#[cfg(all(feature = "backend-mmap", unix, not(feature = "xen")))]
+pub use mmap_unix::{Error as MmapRegionError, GuestMemoryMmap, GuestRegionMmap, MmapRegion};
+
+#[cfg(windows)]
+pub use crate::mmap_windows::{
+    GuestMemoryWindows as GuestMemoryMmap, GuestRegionWindows as GuestRegionMmap, MmapRegion,
+}; // rename for backwards compat
+#[cfg(windows)]
+pub use std::io::Error as MmapRegionError;
+
 pub mod volatile_memory;
 pub use volatile_memory::{
     Error as VolatileMemoryError, Result as VolatileMemoryResult, VolatileArrayRef, VolatileMemory,

src/mmap.rs

@@ -12,30 +12,19 @@
 //!
 //! This implementation is mmap-ing the memory of the guest into the current process.
 
-use std::borrow::Borrow;
 #[cfg(unix)]
 use std::io::{Seek, SeekFrom};
-use std::ops::Deref;
 use std::result;
 
-use crate::address::Address;
-use crate::bitmap::{Bitmap, BS};
-use crate::guest_memory::{self, FileOffset, GuestAddress, GuestUsize, MemoryRegionAddress};
-use crate::region::GuestMemoryRegion;
-use crate::volatile_memory::{VolatileMemory, VolatileSlice};
-use crate::{Error, GuestRegionCollection};
+use crate::bitmap::Bitmap;
+use crate::guest_memory::FileOffset;
 
 #[cfg(all(not(feature = "xen"), unix))]
 pub use crate::mmap_unix::{Error as MmapRegionError, MmapRegion, MmapRegionBuilder};
 
 #[cfg(all(feature = "xen", unix))]
 pub use crate::mmap_xen::{Error as MmapRegionError, MmapRange, MmapRegion, MmapXenFlags};
 
-#[cfg(windows)]
-pub use crate::mmap_windows::MmapRegion;
-#[cfg(windows)]
-pub use std::io::Error as MmapRegionError;
-
 /// A `Bitmap` that can be created starting from an initial size.
 pub trait NewBitmap: Bitmap + Default {
     /// Create a new object based on the specified length in bytes.
@@ -91,166 +80,18 @@ pub fn check_file_offset(
     Ok(())
 }
 
-/// [`GuestMemoryRegion`](trait.GuestMemoryRegion.html) implementation that mmaps the guest's
-/// memory region in the current process.
-///
-/// Represents a continuous region of the guest's physical memory that is backed by a mapping
-/// in the virtual address space of the calling process.
-#[derive(Debug)]
-pub struct GuestRegionMmap<B = ()> {
-    mapping: MmapRegion<B>,
-    guest_base: GuestAddress,
-}
-
-impl<B> Deref for GuestRegionMmap<B> {
-    type Target = MmapRegion<B>;
-
-    fn deref(&self) -> &MmapRegion<B> {
-        &self.mapping
-    }
-}
-
-impl<B: Bitmap> GuestRegionMmap<B> {
-    /// Create a new memory-mapped memory region for the guest's physical memory.
-    pub fn new(mapping: MmapRegion<B>, guest_base: GuestAddress) -> result::Result<Self, Error> {
-        if guest_base.0.checked_add(mapping.size() as u64).is_none() {
-            return Err(Error::InvalidGuestRegion);
-        }
-
-        Ok(GuestRegionMmap {
-            mapping,
-            guest_base,
-        })
-    }
-}
-
-#[cfg(not(feature = "xen"))]
-impl<B: NewBitmap> GuestRegionMmap<B> {
-    /// Create a new memory-mapped memory region from guest's physical memory, size and file.
-    pub fn from_range(
-        addr: GuestAddress,
-        size: usize,
-        file: Option<FileOffset>,
-    ) -> result::Result<Self, Error> {
-        let region = if let Some(ref f_off) = file {
-            MmapRegion::from_file(f_off.clone(), size)
-        } else {
-            MmapRegion::new(size)
-        }
-        .map_err(Error::MmapRegion)?;
-
-        Self::new(region, addr)
-    }
-}
-
-#[cfg(feature = "xen")]
-impl<B: NewBitmap> GuestRegionMmap<B> {
-    /// Create a new Unix memory-mapped memory region from guest's physical memory, size and file.
-    /// This must only be used for tests, doctests, benches and is not designed for end consumers.
-    pub fn from_range(
-        addr: GuestAddress,
-        size: usize,
-        file: Option<FileOffset>,
-    ) -> result::Result<Self, Error> {
-        let range = MmapRange::new_unix(size, file, addr);
-
-        let region = MmapRegion::from_range(range).map_err(Error::MmapRegion)?;
-        Self::new(region, addr)
-    }
-}
-
-impl<B: Bitmap> GuestMemoryRegion for GuestRegionMmap<B> {
-    type B = B;
-
-    fn len(&self) -> GuestUsize {
-        self.mapping.size() as GuestUsize
-    }
-
-    fn start_addr(&self) -> GuestAddress {
-        self.guest_base
-    }
-
-    fn bitmap(&self) -> &Self::B {
-        self.mapping.bitmap()
-    }
-
-    fn get_host_address(&self, addr: MemoryRegionAddress) -> guest_memory::Result<*mut u8> {
-        // Not sure why wrapping_offset is not unsafe.  Anyway this
-        // is safe because we've just range-checked addr using check_address.
-        self.check_address(addr)
-            .ok_or(guest_memory::Error::InvalidBackendAddress)
-            .map(|addr| {
-                self.mapping
-                    .as_ptr()
-                    .wrapping_offset(addr.raw_value() as isize)
-            })
-    }
-
-    fn file_offset(&self) -> Option<&FileOffset> {
-        self.mapping.file_offset()
-    }
-
-    fn get_slice(
-        &self,
-        offset: MemoryRegionAddress,
-        count: usize,
-    ) -> guest_memory::Result<VolatileSlice<BS<B>>> {
-        let slice = VolatileMemory::get_slice(&self.mapping, offset.raw_value() as usize, count)?;
-        Ok(slice)
-    }
-
-    #[cfg(target_os = "linux")]
-    fn is_hugetlbfs(&self) -> Option<bool> {
-        self.mapping.is_hugetlbfs()
-    }
-}
-
-/// [`GuestMemory`](trait.GuestMemory.html) implementation that mmaps the guest's memory
-/// in the current process.
-///
-/// Represents the entire physical memory of the guest by tracking all its memory regions.
-/// Each region is an instance of `GuestRegionMmap`, being backed by a mapping in the
-/// virtual address space of the calling process.
-pub type GuestMemoryMmap<B = ()> = GuestRegionCollection<GuestRegionMmap<B>>;
-
-impl<B: NewBitmap> GuestMemoryMmap<B> {
-    /// Creates a container and allocates anonymous memory for guest memory regions.
-    ///
-    /// Valid memory regions are specified as a slice of (Address, Size) tuples sorted by Address.
-    pub fn from_ranges(ranges: &[(GuestAddress, usize)]) -> result::Result<Self, Error> {
-        Self::from_ranges_with_files(ranges.iter().map(|r| (r.0, r.1, None)))
-    }
-
-    /// Creates a container and allocates anonymous memory for guest memory regions.
-    ///
-    /// Valid memory regions are specified as a sequence of (Address, Size, [`Option<FileOffset>`])
-    /// tuples sorted by Address.
-    pub fn from_ranges_with_files<A, T>(ranges: T) -> result::Result<Self, Error>
-    where
-        A: Borrow<(GuestAddress, usize, Option<FileOffset>)>,
-        T: IntoIterator<Item = A>,
-    {
-        Self::from_regions(
-            ranges
-                .into_iter()
-                .map(|x| {
-                    GuestRegionMmap::from_range(x.borrow().0, x.borrow().1, x.borrow().2.clone())
-                })
-                .collect::<result::Result<Vec<_>, Error>>()?,
-        )
-    }
-}
-
 #[cfg(test)]
 pub(crate) mod tests {
     #![allow(clippy::undocumented_unsafe_blocks)]
     extern crate vmm_sys_util;
 
     use super::*;
 
-    use crate::bitmap::tests::test_guest_memory_and_region;
-    use crate::bitmap::AtomicBitmap;
-    use crate::{Bytes, GuestMemory, GuestMemoryError};
+    use crate::bitmap::BS;
+    use crate::{
+        guest_memory, Address, Bytes, GuestAddress, GuestMemory, GuestMemoryError,
+        GuestMemoryRegion, GuestUsize, MemoryRegionAddress, VolatileMemory, VolatileSlice,
+    };
 
     use std::io::Write;
     use std::mem;
@@ -312,9 +153,9 @@ pub(crate) mod tests {
 
     any_backend! {
         #[cfg(all(windows, feature = "backend-mmap"))]
-        Windows[GuestRegionMmap<()>],
+        Windows[crate::mmap_windows::GuestRegionWindows<()>],
         #[cfg(all(unix, feature = "backend-mmap", not(feature = "xen")))]
-        Mmap[GuestRegionMmap<()>],
+        Mmap[crate::mmap_unix::GuestRegionMmap<()>],
         #[cfg(all(unix, feature = "backend-mmap", feature = "xen"))]
         Xen[crate::mmap_xen::MmapRegion]
     }
@@ -326,13 +167,19 @@ pub(crate) mod tests {
             let mut regions = Vec::new();
             #[cfg(all(windows, feature = "backend-mmap"))]
             regions.push(AnyRegion::Windows(
-                GuestRegionMmap::new(MmapRegion::from_file(f_off.clone(), size).unwrap(), addr)
-                    .unwrap(),
+                crate::mmap_windows::GuestRegionWindows::new(
+                    crate::mmap_windows::MmapRegion::from_file(f_off.clone(), size).unwrap(),
+                    addr,
+                )
+                .unwrap(),
             ));
             #[cfg(all(unix, feature = "backend-mmap", not(feature = "xen")))]
             regions.push(AnyRegion::Mmap(
-                GuestRegionMmap::new(MmapRegion::from_file(f_off.clone(), size).unwrap(), addr)
-                    .unwrap(),
+                crate::mmap_unix::GuestRegionMmap::new(
+                    MmapRegion::from_file(f_off.clone(), size).unwrap(),
+                    addr,
+                )
+                .unwrap(),
             ));
             #[cfg(all(unix, feature = "backend-mmap", feature = "xen"))]
             regions.push(AnyRegion::Xen(
@@ -350,11 +197,16 @@ pub(crate) mod tests {
             };
             #[cfg(all(windows, feature = "backend-mmap"))]
             regions.push(AnyRegion::Windows(
-                GuestRegionMmap::new(MmapRegion::new(size).unwrap(), addr).unwrap(),
+                crate::mmap_windows::GuestRegionWindows::new(
+                    crate::mmap_windows::MmapRegion::new(size).unwrap(),
+                    addr,
+                )
+                .unwrap(),
             ));
             #[cfg(all(unix, feature = "backend-mmap", not(feature = "xen")))]
             regions.push(AnyRegion::Mmap(
-                GuestRegionMmap::new(MmapRegion::new(size).unwrap(), addr).unwrap(),
+                crate::mmap_unix::GuestRegionMmap::new(MmapRegion::new(size).unwrap(), addr)
+                    .unwrap(),
             ));
             #[cfg(all(unix, feature = "backend-mmap", feature = "xen"))]
             regions.push(AnyRegion::Xen(
@@ -731,12 +583,4 @@ pub(crate) mod tests {
             );
         }
     }
-
-    #[test]
-    fn test_dirty_tracking() {
-        test_guest_memory_and_region(|| {
-            crate::GuestMemoryMmap::<AtomicBitmap>::from_ranges(&[(GuestAddress(0), 0x1_0000)])
-                .unwrap()
-        });
-    }
 }