refactor: clean up region collection related errors

Turn the error type defined in region.rs into one specifically to
GuestRegionCollections, and move the non-collection related errors into
a new error type that is specific to the `from_ranges*` family of
functions (these are the only functions that can return both a region
construction error, as well as a collection error). The `from_ranges*`
family of functions is publicly exported, but in reality only used in
tests, so breakage of this should be limited.

Since we're breaking things anyway, remove the reexport of
GuestRegionCollectionError as `vm_memory::Error`. It made no sense for
this one to have this special designation.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Author: roypat

src/lib.rs

@@ -52,7 +52,7 @@ pub use guest_memory::{
 
 pub mod region;
 pub use region::{
-    GuestMemoryRegion, GuestMemoryRegionBytes, GuestRegionCollection, GuestRegionError as Error,
+    GuestMemoryRegion, GuestMemoryRegionBytes, GuestRegionCollection, GuestRegionCollectionError,
 };
 
 pub mod io;

src/mmap/mod.rs

@@ -19,9 +19,10 @@ 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, GuestMemoryRegionBytes};
+use crate::region::{
+    GuestMemoryRegion, GuestMemoryRegionBytes, GuestRegionCollection, GuestRegionCollectionError,
+};
 use crate::volatile_memory::{VolatileMemory, VolatileSlice};
-use crate::{Error, GuestRegionCollection};
 
 // re-export for backward compat, as the trait used to be defined in mmap.rs
 pub use crate::bitmap::NewBitmap;
@@ -87,15 +88,14 @@ impl<B: NewBitmap> GuestRegionMmap<B> {
         addr: GuestAddress,
         size: usize,
         file: Option<FileOffset>,
-    ) -> result::Result<Self, Error> {
+    ) -> result::Result<Self, FromRangesError> {
         let region = if let Some(ref f_off) = file {
-            MmapRegion::from_file(f_off.clone(), size)
+            MmapRegion::from_file(f_off.clone(), size)?
         } else {
-            MmapRegion::new(size)
-        }
-        .map_err(Error::MmapRegion)?;
+            MmapRegion::new(size)?
+        };
 
-        Self::new(region, addr).ok_or(Error::InvalidGuestRegion)
+        Self::new(region, addr).ok_or(FromRangesError::InvalidGuestRegion)
     }
 }
 
@@ -107,11 +107,11 @@ impl<B: NewBitmap> GuestRegionMmap<B> {
         addr: GuestAddress,
         size: usize,
         file: Option<FileOffset>,
-    ) -> result::Result<Self, Error> {
+    ) -> result::Result<Self, FromRangesError> {
         let range = MmapRange::new_unix(size, file, addr);
 
-        let region = MmapRegion::from_range(range).map_err(Error::MmapRegion)?;
-        Self::new(region, addr).ok_or(Error::InvalidGuestRegion)
+        let region = MmapRegion::from_range(range)?;
+        Self::new(region, addr).ok_or(FromRangesError::InvalidGuestRegion)
     }
 }
 
@@ -171,19 +171,33 @@ impl<B: Bitmap> GuestMemoryRegionBytes for GuestRegionMmap<B> {}
 /// virtual address space of the calling process.
 pub type GuestMemoryMmap<B = ()> = GuestRegionCollection<GuestRegionMmap<B>>;
 
+/// Errors that can happen during [`GuestMemoryMap::from_ranges`] and related functions.
+#[derive(Debug, thiserror::Error)]
+pub enum FromRangesError {
+    /// Error during construction of [`GuestMemoryMmap`]
+    #[error("Error constructing guest region collection: {0}")]
+    Collection(#[from] GuestRegionCollectionError),
+    /// Error while allocating raw mmap region
+    #[error("Error setting up raw memory for guest region: {0}")]
+    MmapRegion(#[from] MmapRegionError),
+    /// A combination of region length and guest address would overflow.
+    #[error("Combination of guest address and region length invalid (would overflow)")]
+    InvalidGuestRegion,
+}
+
 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> {
+    pub fn from_ranges(ranges: &[(GuestAddress, usize)]) -> result::Result<Self, FromRangesError> {
         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>
+    pub fn from_ranges_with_files<A, T>(ranges: T) -> result::Result<Self, FromRangesError>
     where
         A: Borrow<(GuestAddress, usize, Option<FileOffset>)>,
         T: IntoIterator<Item = A>,
@@ -194,8 +208,9 @@ impl<B: NewBitmap> GuestMemoryMmap<B> {
                 .map(|x| {
                     GuestRegionMmap::from_range(x.borrow().0, x.borrow().1, x.borrow().2.clone())
                 })
-                .collect::<result::Result<Vec<_>, Error>>()?,
+                .collect::<Result<Vec<_>, _>>()?,
         )
+        .map_err(Into::into)
     }
 }
 

src/region.rs

@@ -169,18 +169,9 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = GuestMemoryError> {
     }
 }
 
-/// Errors that can occur when dealing with [`GuestRegion`]s, or collections thereof
+/// Errors that can occur when dealing with [`GuestRegionCollection`]s
 #[derive(Debug, thiserror::Error)]
-pub enum GuestRegionError {
-    /// Adding the guest base address to the length of the underlying mapping resulted
-    /// in an overflow.
-    #[error("Adding the guest base address to the length of the underlying mapping resulted in an overflow")]
-    #[cfg(feature = "backend-mmap")]
-    InvalidGuestRegion,
-    /// Error creating a `MmapRegion` object.
-    #[error("{0}")]
-    #[cfg(feature = "backend-mmap")]
-    MmapRegion(crate::mmap::MmapRegionError),
+pub enum GuestRegionCollectionError {
     /// No memory region found.
     #[error("No memory region found")]
     NoMemoryRegion,
@@ -230,7 +221,9 @@ impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
     /// * `regions` - The vector of regions.
     ///               The regions shouldn't overlap, and they should be sorted
     ///               by the starting address.
-    pub fn from_regions(mut regions: Vec<R>) -> std::result::Result<Self, GuestRegionError> {
+    pub fn from_regions(
+        mut regions: Vec<R>,
+    ) -> std::result::Result<Self, GuestRegionCollectionError> {
         Self::from_arc_regions(regions.drain(..).map(Arc::new).collect())
     }
 
@@ -246,21 +239,23 @@ impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
     /// * `regions` - The vector of `Arc` regions.
     ///               The regions shouldn't overlap and they should be sorted
     ///               by the starting address.
-    pub fn from_arc_regions(regions: Vec<Arc<R>>) -> std::result::Result<Self, GuestRegionError> {
+    pub fn from_arc_regions(
+        regions: Vec<Arc<R>>,
+    ) -> std::result::Result<Self, GuestRegionCollectionError> {
         if regions.is_empty() {
-            return Err(GuestRegionError::NoMemoryRegion);
+            return Err(GuestRegionCollectionError::NoMemoryRegion);
         }
 
         for window in regions.windows(2) {
             let prev = &window[0];
             let next = &window[1];
 
             if prev.start_addr() > next.start_addr() {
-                return Err(GuestRegionError::UnsortedMemoryRegions);
+                return Err(GuestRegionCollectionError::UnsortedMemoryRegions);
             }
 
             if prev.last_addr() >= next.start_addr() {
-                return Err(GuestRegionError::MemoryRegionOverlap);
+                return Err(GuestRegionCollectionError::MemoryRegionOverlap);
             }
         }
 
@@ -274,7 +269,7 @@ impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
     pub fn insert_region(
         &self,
         region: Arc<R>,
-    ) -> std::result::Result<GuestRegionCollection<R>, GuestRegionError> {
+    ) -> std::result::Result<GuestRegionCollection<R>, GuestRegionCollectionError> {
         let mut regions = self.regions.clone();
         regions.push(region);
         regions.sort_by_key(|x| x.start_addr());
@@ -292,7 +287,7 @@ impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
         &self,
         base: GuestAddress,
         size: GuestUsize,
-    ) -> std::result::Result<(GuestRegionCollection<R>, Arc<R>), GuestRegionError> {
+    ) -> std::result::Result<(GuestRegionCollection<R>, Arc<R>), GuestRegionCollectionError> {
         if let Ok(region_index) = self.regions.binary_search_by_key(&base, |x| x.start_addr()) {
             if self.regions.get(region_index).unwrap().len() == size {
                 let mut regions = self.regions.clone();
@@ -301,7 +296,7 @@ impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
             }
         }
 
-        Err(GuestRegionError::NoMemoryRegion)
+        Err(GuestRegionCollectionError::NoMemoryRegion)
     }
 }
 
@@ -479,7 +474,7 @@ impl<R: GuestMemoryRegionBytes> Bytes<MemoryRegionAddress> for R {
 
 #[cfg(test)]
 pub(crate) mod tests {
-    use crate::region::{GuestMemoryRegionBytes, GuestRegionError};
+    use crate::region::{GuestMemoryRegionBytes, GuestRegionCollectionError};
     use crate::{
         Address, GuestAddress, GuestMemory, GuestMemoryRegion, GuestRegionCollection, GuestUsize,
     };
@@ -510,7 +505,7 @@ pub(crate) mod tests {
     pub(crate) type Collection = GuestRegionCollection<MockRegion>;
 
     fn check_guest_memory_mmap(
-        maybe_guest_mem: Result<Collection, GuestRegionError>,
+        maybe_guest_mem: Result<Collection, GuestRegionCollectionError>,
         expected_regions_summary: &[(GuestAddress, u64)],
     ) {
         assert!(maybe_guest_mem.is_ok());
@@ -537,7 +532,7 @@ pub(crate) mod tests {
 
     pub(crate) fn new_guest_memory_collection_from_regions(
         regions_summary: &[(GuestAddress, u64)],
-    ) -> Result<Collection, GuestRegionError> {
+    ) -> Result<Collection, GuestRegionCollectionError> {
         Collection::from_regions(
             regions_summary
                 .iter()
@@ -548,7 +543,7 @@ pub(crate) mod tests {
 
     fn new_guest_memory_collection_from_arc_regions(
         regions_summary: &[(GuestAddress, u64)],
-    ) -> Result<Collection, GuestRegionError> {
+    ) -> Result<Collection, GuestRegionCollectionError> {
         Collection::from_arc_regions(
             regions_summary
                 .iter()
@@ -563,11 +558,11 @@ pub(crate) mod tests {
 
         assert!(matches!(
             new_guest_memory_collection_from_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::NoMemoryRegion
+            GuestRegionCollectionError::NoMemoryRegion
         ));
         assert!(matches!(
             new_guest_memory_collection_from_arc_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::NoMemoryRegion
+            GuestRegionCollectionError::NoMemoryRegion
         ));
     }
 
@@ -577,11 +572,11 @@ pub(crate) mod tests {
 
         assert!(matches!(
             new_guest_memory_collection_from_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::MemoryRegionOverlap
+            GuestRegionCollectionError::MemoryRegionOverlap
         ));
         assert!(matches!(
             new_guest_memory_collection_from_arc_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::MemoryRegionOverlap
+            GuestRegionCollectionError::MemoryRegionOverlap
         ));
     }
 
@@ -591,11 +586,11 @@ pub(crate) mod tests {
 
         assert!(matches!(
             new_guest_memory_collection_from_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::UnsortedMemoryRegions
+            GuestRegionCollectionError::UnsortedMemoryRegions
         ));
         assert!(matches!(
             new_guest_memory_collection_from_arc_regions(&regions_summary).unwrap_err(),
-            GuestRegionError::UnsortedMemoryRegions
+            GuestRegionCollectionError::UnsortedMemoryRegions
         ));
     }