From 85275159e4a30b10e363d6a7e3fac76c7efe76de Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 5 Feb 2025 07:48:37 +0000 Subject: [PATCH 1/3] Add naive default impl for `GuestMemory::find_region()` This function can be default-implemented in terms of `GuestMemory::iter()`. Downstream impls can overwrite this more specialized and efficient versions of course (such as GuestMemoryMmap using a binary search). Signed-off-by: Patrick Roy --- src/guest_memory.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 189ea8dd..9ece9ae0 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -408,7 +408,9 @@ pub trait GuestMemory { fn num_regions(&self) -> usize; /// Returns the region containing the specified address or `None`. - fn find_region(&self, addr: GuestAddress) -> Option<&Self::R>; + fn find_region(&self, addr: GuestAddress) -> Option<&Self::R> { + self.iter().find(|region| addr >= region.start_addr() && addr <= region.last_addr()) + } /// Gets an iterator over the entries in the collection. /// From b31bfd0e0894531a2822e673123fb307b6ea50a1 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 5 Feb 2025 11:29:53 +0000 Subject: [PATCH 2/3] Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions Add the concept of a `GuestRegionCollection`, which just manages a list of some `GuestMemoryRegion` impls. Functionality wise, it offers the same implementations as `GuestMemoryMmap`. As a result, turn `GuestMemoryMmap` into a type alias for `GuestRegionCollection` with a fixed `R = GuestRegionMmap`. The error type handling is a bit wack, but this is needed to preserve backwards compatibility: The error type of GuestRegionCollection must match what GuestMemoryMmap historically returned, so the error type needs to be lifted from mmap.rs - however, it contains specific variants that are only relevant to GuestMemoryMmap, so cfg those behind the `backend-mmap` feature flag (as to why this specific error type gets be privilege of just being reexported as `Error` from this crate: No idea, but its pre-existing, and changing it would be a breaking change). Signed-off-by: Patrick Roy --- CHANGELOG.md | 2 + src/guest_memory.rs | 165 +++++++++++++++++++++++++++++++++++++++- src/lib.rs | 5 +- src/mmap.rs | 180 +++++++------------------------------------- 4 files changed, 195 insertions(+), 157 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1881e7bb..6934c352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Added - \[[#311](https://github.com/rust-vmm/vm-memory/pull/311)\] Allow compiling without the ReadVolatile and WriteVolatile implementations +- \[[#312](https://github.com/rust-vmm/vm-memory/pull/312)\] `GuestRegionContainer`, a generic container of `GuestMemoryRegion`s, generalizing `GuestMemoryMmap` (which + is now a type alias for `GuestRegionContainer`) ### Changed diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 9ece9ae0..c45d764c 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -409,7 +409,8 @@ pub trait GuestMemory { /// Returns the region containing the specified address or `None`. fn find_region(&self, addr: GuestAddress) -> Option<&Self::R> { - self.iter().find(|region| addr >= region.start_addr() && addr <= region.last_addr()) + self.iter() + .find(|region| addr >= region.start_addr() && addr <= region.last_addr()) } /// Gets an iterator over the entries in the collection. @@ -586,6 +587,168 @@ pub trait GuestMemory { } } +/// Errors that can occur when creating a memory map. +#[derive(Debug, thiserror::Error)] +pub enum GuestRegionCollectionError { + /// 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), + /// No memory region found. + #[error("No memory region found")] + NoMemoryRegion, + /// Some of the memory regions intersect with each other. + #[error("Some of the memory regions intersect with each other")] + MemoryRegionOverlap, + /// The provided memory regions haven't been sorted. + #[error("The provided memory regions haven't been sorted")] + UnsortedMemoryRegions, +} + +/// [`GuestMemory`](trait.GuestMemory.html) implementation based on a homogeneous collection +/// of [`GuestMemoryRegion`] implementations. +/// +/// Represents a sorted set of non-overlapping physical guest memory regions. +#[derive(Debug)] +pub struct GuestRegionCollection { + regions: Vec>, +} + +impl Default for GuestRegionCollection { + fn default() -> Self { + Self { + regions: Vec::new(), + } + } +} + +impl Clone for GuestRegionCollection { + fn clone(&self) -> Self { + GuestRegionCollection { + regions: self.regions.iter().map(Arc::clone).collect(), + } + } +} + +impl GuestRegionCollection { + /// Creates an empty `GuestMemoryMmap` instance. + pub fn new() -> Self { + Self::default() + } + + /// Creates a new [`GuestRegionCollection`] from a vector of regions. + /// + /// # Arguments + /// + /// * `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, + ) -> std::result::Result { + Self::from_arc_regions(regions.drain(..).map(Arc::new).collect()) + } + + /// Creates a new [`GuestRegionCollection`] from a vector of Arc regions. + /// + /// Similar to the constructor `from_regions()` as it returns a + /// [`GuestRegionCollection`]. The need for this constructor is to provide a way for + /// consumer of this API to create a new [`GuestRegionCollection`] based on existing + /// regions coming from an existing [`GuestRegionCollection`] instance. + /// + /// # Arguments + /// + /// * `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>, + ) -> std::result::Result { + if regions.is_empty() { + 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(GuestRegionCollectionError::UnsortedMemoryRegions); + } + + if prev.last_addr() >= next.start_addr() { + return Err(GuestRegionCollectionError::MemoryRegionOverlap); + } + } + + Ok(Self { regions }) + } + + /// Insert a region into the `GuestMemoryMmap` object and return a new `GuestMemoryMmap`. + /// + /// # Arguments + /// * `region`: the memory region to insert into the guest memory object. + pub fn insert_region( + &self, + region: Arc, + ) -> std::result::Result, GuestRegionCollectionError> { + let mut regions = self.regions.clone(); + regions.push(region); + regions.sort_by_key(|x| x.start_addr()); + + Self::from_arc_regions(regions) + } + + /// Remove a region from the [`GuestRegionCollection`] object and return a new `GuestRegionCollection` + /// on success, together with the removed region. + /// + /// # Arguments + /// * `base`: base address of the region to be removed + /// * `size`: size of the region to be removed + pub fn remove_region( + &self, + base: GuestAddress, + size: GuestUsize, + ) -> std::result::Result<(GuestRegionCollection, Arc), 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(); + let region = regions.remove(region_index); + return Ok((Self { regions }, region)); + } + } + + Err(GuestRegionCollectionError::NoMemoryRegion) + } +} + +impl GuestMemory for GuestRegionCollection { + type R = R; + + fn num_regions(&self) -> usize { + self.regions.len() + } + + fn find_region(&self, addr: GuestAddress) -> Option<&R> { + let index = match self.regions.binary_search_by_key(&addr, |x| x.start_addr()) { + Ok(x) => Some(x), + // Within the closest region with starting address < addr + Err(x) if (x > 0 && addr <= self.regions[x - 1].last_addr()) => Some(x - 1), + _ => None, + }; + index.map(|x| self.regions[x].as_ref()) + } + + fn iter(&self) -> impl Iterator { + self.regions.iter().map(AsRef::as_ref) + } +} + impl Bytes for T { type E = Error; diff --git a/src/lib.rs b/src/lib.rs index 6f87ce48..d796e9d2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,7 +47,8 @@ pub use endian::{Be16, Be32, Be64, BeSize, Le16, Le32, Le64, LeSize}; pub mod guest_memory; pub use guest_memory::{ Error as GuestMemoryError, FileOffset, GuestAddress, GuestAddressSpace, GuestMemory, - GuestMemoryRegion, GuestUsize, MemoryRegionAddress, Result as GuestMemoryResult, + GuestMemoryRegion, GuestRegionCollection, GuestRegionCollectionError as Error, GuestUsize, + MemoryRegionAddress, Result as GuestMemoryResult, }; pub mod io; @@ -66,7 +67,7 @@ mod mmap_windows; pub mod mmap; #[cfg(feature = "backend-mmap")] -pub use mmap::{Error, GuestMemoryMmap, GuestRegionMmap, MmapRegion}; +pub use mmap::{GuestMemoryMmap, GuestRegionMmap, MmapRegion}; #[cfg(all(feature = "backend-mmap", feature = "xen", unix))] pub use mmap::{MmapRange, MmapXenFlags}; diff --git a/src/mmap.rs b/src/mmap.rs index a5c81f24..327f9666 100644 --- a/src/mmap.rs +++ b/src/mmap.rs @@ -18,15 +18,15 @@ use std::io::{Seek, SeekFrom}; use std::ops::Deref; use std::result; use std::sync::atomic::Ordering; -use std::sync::Arc; use crate::address::Address; use crate::bitmap::{Bitmap, BS}; use crate::guest_memory::{ - self, FileOffset, GuestAddress, GuestMemory, GuestMemoryRegion, GuestUsize, MemoryRegionAddress, + self, FileOffset, GuestAddress, GuestMemoryRegion, GuestRegionCollection, GuestUsize, + MemoryRegionAddress, }; use crate::volatile_memory::{VolatileMemory, VolatileSlice}; -use crate::{AtomicAccess, Bytes, ReadVolatile, WriteVolatile}; +use crate::{AtomicAccess, Bytes, Error, ReadVolatile, WriteVolatile}; #[cfg(all(not(feature = "xen"), unix))] pub use crate::mmap_unix::{Error as MmapRegionError, MmapRegion, MmapRegionBuilder}; @@ -49,27 +49,6 @@ impl NewBitmap for () { fn with_len(_len: usize) -> Self {} } -/// Errors that can occur when creating a memory map. -#[derive(Debug, thiserror::Error)] -pub enum Error { - /// 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")] - InvalidGuestRegion, - /// Error creating a `MmapRegion` object. - #[error("{0}")] - MmapRegion(MmapRegionError), - /// No memory region found. - #[error("No memory region found")] - NoMemoryRegion, - /// Some of the memory regions intersect with each other. - #[error("Some of the memory regions intersect with each other")] - MemoryRegionOverlap, - /// The provided memory regions haven't been sorted. - #[error("The provided memory regions haven't been sorted")] - UnsortedMemoryRegions, -} - // TODO: use this for Windows as well after we redefine the Error type there. #[cfg(unix)] /// Checks if a mapping of `size` bytes fits at the provided `file_offset`. @@ -366,17 +345,9 @@ impl GuestMemoryRegion for GuestRegionMmap { /// 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. -#[derive(Clone, Debug, Default)] -pub struct GuestMemoryMmap { - regions: Vec>>, -} +pub type GuestMemoryMmap = GuestRegionCollection>; impl GuestMemoryMmap { - /// Creates an empty `GuestMemoryMmap` instance. - pub fn new() -> Self { - Self::default() - } - /// 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. @@ -404,111 +375,6 @@ impl GuestMemoryMmap { } } -impl GuestMemoryMmap { - /// Creates a new `GuestMemoryMmap` from a vector of regions. - /// - /// # Arguments - /// - /// * `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>) -> result::Result { - Self::from_arc_regions(regions.drain(..).map(Arc::new).collect()) - } - - /// Creates a new `GuestMemoryMmap` from a vector of Arc regions. - /// - /// Similar to the constructor `from_regions()` as it returns a - /// `GuestMemoryMmap`. The need for this constructor is to provide a way for - /// consumer of this API to create a new `GuestMemoryMmap` based on existing - /// regions coming from an existing `GuestMemoryMmap` instance. - /// - /// # Arguments - /// - /// * `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>>) -> result::Result { - if regions.is_empty() { - return Err(Error::NoMemoryRegion); - } - - for window in regions.windows(2) { - let prev = &window[0]; - let next = &window[1]; - - if prev.start_addr() > next.start_addr() { - return Err(Error::UnsortedMemoryRegions); - } - - if prev.last_addr() >= next.start_addr() { - return Err(Error::MemoryRegionOverlap); - } - } - - Ok(Self { regions }) - } - - /// Insert a region into the `GuestMemoryMmap` object and return a new `GuestMemoryMmap`. - /// - /// # Arguments - /// * `region`: the memory region to insert into the guest memory object. - pub fn insert_region( - &self, - region: Arc>, - ) -> result::Result, Error> { - let mut regions = self.regions.clone(); - regions.push(region); - regions.sort_by_key(|x| x.start_addr()); - - Self::from_arc_regions(regions) - } - - /// Remove a region into the `GuestMemoryMmap` object and return a new `GuestMemoryMmap` - /// on success, together with the removed region. - /// - /// # Arguments - /// * `base`: base address of the region to be removed - /// * `size`: size of the region to be removed - pub fn remove_region( - &self, - base: GuestAddress, - size: GuestUsize, - ) -> result::Result<(GuestMemoryMmap, Arc>), Error> { - if let Ok(region_index) = self.regions.binary_search_by_key(&base, |x| x.start_addr()) { - if self.regions.get(region_index).unwrap().mapping.size() as GuestUsize == size { - let mut regions = self.regions.clone(); - let region = regions.remove(region_index); - return Ok((Self { regions }, region)); - } - } - - Err(Error::InvalidGuestRegion) - } -} - -impl GuestMemory for GuestMemoryMmap { - type R = GuestRegionMmap; - - fn num_regions(&self) -> usize { - self.regions.len() - } - - fn find_region(&self, addr: GuestAddress) -> Option<&GuestRegionMmap> { - let index = match self.regions.binary_search_by_key(&addr, |x| x.start_addr()) { - Ok(x) => Some(x), - // Within the closest region with starting address < addr - Err(x) if (x > 0 && addr <= self.regions[x - 1].last_addr()) => Some(x - 1), - _ => None, - }; - index.map(|x| self.regions[x].as_ref()) - } - - fn iter(&self) -> impl Iterator { - self.regions.iter().map(AsRef::as_ref) - } -} - #[cfg(test)] mod tests { #![allow(clippy::undocumented_unsafe_blocks)] @@ -518,16 +384,17 @@ mod tests { use crate::bitmap::tests::test_guest_memory_and_region; use crate::bitmap::AtomicBitmap; - use crate::GuestAddressSpace; + use crate::{Error, GuestAddressSpace, GuestMemory}; use std::io::Write; use std::mem; + use std::sync::Arc; #[cfg(feature = "rawfd")] use std::{fs::File, path::Path}; use vmm_sys_util::tempfile::TempFile; - type GuestMemoryMmap = super::GuestMemoryMmap<()>; type GuestRegionMmap = super::GuestRegionMmap<()>; + type GuestMemoryMmap = super::GuestRegionCollection; type MmapRegion = super::MmapRegion<()>; #[test] @@ -552,9 +419,8 @@ mod tests { } assert_eq!(guest_mem.last_addr(), last_addr); } - for ((region_addr, region_size), mmap) in expected_regions_summary - .iter() - .zip(guest_mem.regions.iter()) + for ((region_addr, region_size), mmap) in + expected_regions_summary.iter().zip(guest_mem.iter()) { assert_eq!(region_addr, &mmap.guest_base); assert_eq!(region_size, &mmap.mapping.size()); @@ -745,7 +611,7 @@ mod tests { let regions_summary = [(GuestAddress(0), 100_usize), (GuestAddress(100), 100_usize)]; let guest_mem = GuestMemoryMmap::new(); - assert_eq!(guest_mem.regions.len(), 0); + assert_eq!(guest_mem.num_regions(), 0); check_guest_memory_mmap(new_guest_memory_mmap(®ions_summary), ®ions_summary); @@ -1083,8 +949,10 @@ mod tests { .map(|x| (x.0, x.1)) .eq(iterated_regions.iter().copied())); - assert_eq!(gm.regions[0].guest_base, regions[0].0); - assert_eq!(gm.regions[1].guest_base, regions[1].0); + let mmap_regions = gm.iter().collect::>(); + + assert_eq!(mmap_regions[0].guest_base, regions[0].0); + assert_eq!(mmap_regions[1].guest_base, regions[1].0); } #[test] @@ -1112,8 +980,10 @@ mod tests { .map(|x| (x.0, x.1)) .eq(iterated_regions.iter().copied())); - assert_eq!(gm.regions[0].guest_base, regions[0].0); - assert_eq!(gm.regions[1].guest_base, regions[1].0); + let mmap_regions = gm.iter().collect::>(); + + assert_eq!(mmap_regions[0].guest_base, regions[0].0); + assert_eq!(mmap_regions[1].guest_base, regions[1].0); } #[test] @@ -1222,11 +1092,13 @@ mod tests { assert_eq!(mem_orig.num_regions(), 2); assert_eq!(gm.num_regions(), 5); - assert_eq!(gm.regions[0].start_addr(), GuestAddress(0x0000)); - assert_eq!(gm.regions[1].start_addr(), GuestAddress(0x4000)); - assert_eq!(gm.regions[2].start_addr(), GuestAddress(0x8000)); - assert_eq!(gm.regions[3].start_addr(), GuestAddress(0xc000)); - assert_eq!(gm.regions[4].start_addr(), GuestAddress(0x10_0000)); + let regions = gm.iter().collect::>(); + + assert_eq!(regions[0].start_addr(), GuestAddress(0x0000)); + assert_eq!(regions[1].start_addr(), GuestAddress(0x4000)); + assert_eq!(regions[2].start_addr(), GuestAddress(0x8000)); + assert_eq!(regions[3].start_addr(), GuestAddress(0xc000)); + assert_eq!(regions[4].start_addr(), GuestAddress(0x10_0000)); } #[test] @@ -1247,7 +1119,7 @@ mod tests { assert_eq!(mem_orig.num_regions(), 2); assert_eq!(gm.num_regions(), 1); - assert_eq!(gm.regions[0].start_addr(), GuestAddress(0x0000)); + assert_eq!(gm.iter().next().unwrap().start_addr(), GuestAddress(0x0000)); assert_eq!(region.start_addr(), GuestAddress(0x10_0000)); } From c76a6ca84c794ee5a6d390c3246f2bf94216fd57 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 5 Feb 2025 13:25:13 +0000 Subject: [PATCH 3/3] refactor: use `matches!` instead of to_string() for tests Some tests that were explicitly testing for error conditions used converted errors to strings to determine whether two errors are the same (by saying they're only the same if their string representation was identical). Replace this with more roboust assertions on `matches!`. Signed-off-by: Patrick Roy --- src/mmap.rs | 180 +++++++++++++---------------------------- src/mmap_unix.rs | 8 +- src/mmap_xen.rs | 30 +++---- src/volatile_memory.rs | 52 ------------ 4 files changed, 71 insertions(+), 199 deletions(-) diff --git a/src/mmap.rs b/src/mmap.rs index 327f9666..6f0faf7f 100644 --- a/src/mmap.rs +++ b/src/mmap.rs @@ -384,7 +384,7 @@ mod tests { use crate::bitmap::tests::test_guest_memory_and_region; use crate::bitmap::AtomicBitmap; - use crate::{Error, GuestAddressSpace, GuestMemory}; + use crate::{Error, GuestAddressSpace, GuestMemory, GuestMemoryError}; use std::io::Write; use std::mem; @@ -481,129 +481,66 @@ mod tests { fn test_no_memory_region() { let regions_summary = []; - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap(®ions_summary).err().unwrap() - ), - format!("{:?}", Error::NoMemoryRegion) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_with_files(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::NoMemoryRegion) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::NoMemoryRegion) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_arc_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::NoMemoryRegion) - ); + assert!(matches!( + new_guest_memory_mmap(®ions_summary).unwrap_err(), + Error::NoMemoryRegion + )); + assert!(matches!( + new_guest_memory_mmap_with_files(®ions_summary).unwrap_err(), + Error::NoMemoryRegion + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::NoMemoryRegion + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::NoMemoryRegion + )); } #[test] fn test_overlapping_memory_regions() { let regions_summary = [(GuestAddress(0), 100_usize), (GuestAddress(99), 100_usize)]; - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap(®ions_summary).err().unwrap() - ), - format!("{:?}", Error::MemoryRegionOverlap) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_with_files(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::MemoryRegionOverlap) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::MemoryRegionOverlap) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_arc_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::MemoryRegionOverlap) - ); + assert!(matches!( + new_guest_memory_mmap(®ions_summary).unwrap_err(), + Error::MemoryRegionOverlap + )); + assert!(matches!( + new_guest_memory_mmap_with_files(®ions_summary).unwrap_err(), + Error::MemoryRegionOverlap + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::MemoryRegionOverlap + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::MemoryRegionOverlap + )); } #[test] fn test_unsorted_memory_regions() { let regions_summary = [(GuestAddress(100), 100_usize), (GuestAddress(0), 100_usize)]; - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap(®ions_summary).err().unwrap() - ), - format!("{:?}", Error::UnsortedMemoryRegions) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_with_files(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::UnsortedMemoryRegions) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::UnsortedMemoryRegions) - ); - - assert_eq!( - format!( - "{:?}", - new_guest_memory_mmap_from_arc_regions(®ions_summary) - .err() - .unwrap() - ), - format!("{:?}", Error::UnsortedMemoryRegions) - ); + assert!(matches!( + new_guest_memory_mmap(®ions_summary).unwrap_err(), + Error::UnsortedMemoryRegions + )); + assert!(matches!( + new_guest_memory_mmap_with_files(®ions_summary).unwrap_err(), + Error::UnsortedMemoryRegions + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::UnsortedMemoryRegions + )); + assert!(matches!( + new_guest_memory_mmap_from_regions(®ions_summary).unwrap_err(), + Error::UnsortedMemoryRegions + )); } #[test] @@ -828,18 +765,13 @@ mod tests { for gm in gm_list.iter() { let val1: u64 = 0xaa55_aa55_aa55_aa55; let val2: u64 = 0x55aa_55aa_55aa_55aa; - assert_eq!( - format!("{:?}", gm.write_obj(val1, bad_addr).err().unwrap()), - format!("InvalidGuestAddress({:?})", bad_addr,) - ); - assert_eq!( - format!("{:?}", gm.write_obj(val1, bad_addr2).err().unwrap()), - format!( - "PartialBuffer {{ expected: {:?}, completed: {:?} }}", - mem::size_of::(), - max_addr.checked_offset_from(bad_addr2).unwrap() - ) - ); + assert!(matches!( + gm.write_obj(val1, bad_addr).unwrap_err(), + GuestMemoryError::InvalidGuestAddress(addr) if addr == bad_addr + )); + assert!(matches!( + gm.write_obj(val1, bad_addr2).unwrap_err(), + GuestMemoryError::PartialBuffer { expected, completed} if expected == size_of::() && completed == max_addr.checked_offset_from(bad_addr2).unwrap() as usize)); gm.write_obj(val1, GuestAddress(0x500)).unwrap(); gm.write_obj(val2, GuestAddress(0x1000 + 32)).unwrap(); diff --git a/src/mmap_unix.rs b/src/mmap_unix.rs index 14ceb809..667c7f9b 100644 --- a/src/mmap_unix.rs +++ b/src/mmap_unix.rs @@ -558,7 +558,7 @@ mod tests { prot, flags, ); - assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength"); + assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength)); // Offset + size is greater than the size of the file (which is 0 at this point). let r = MmapRegion::build( @@ -567,7 +567,7 @@ mod tests { prot, flags, ); - assert_eq!(format!("{:?}", r.unwrap_err()), "MappingPastEof"); + assert!(matches!(r.unwrap_err(), Error::MappingPastEof)); // MAP_FIXED was specified among the flags. let r = MmapRegion::build( @@ -576,7 +576,7 @@ mod tests { prot, flags | libc::MAP_FIXED, ); - assert_eq!(format!("{:?}", r.unwrap_err()), "MapFixed"); + assert!(matches!(r.unwrap_err(), Error::MapFixed)); // Let's resize the file. assert_eq!(unsafe { libc::ftruncate(a.as_raw_fd(), 1024 * 10) }, 0); @@ -621,7 +621,7 @@ mod tests { let flags = libc::MAP_NORESERVE | libc::MAP_PRIVATE; let r = unsafe { MmapRegion::build_raw((addr + 1) as *mut u8, size, prot, flags) }; - assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidPointer"); + assert!(matches!(r.unwrap_err(), Error::InvalidPointer)); let r = unsafe { MmapRegion::build_raw(addr as *mut u8, size, prot, flags).unwrap() }; diff --git a/src/mmap_xen.rs b/src/mmap_xen.rs index b49495a3..b88db594 100644 --- a/src/mmap_xen.rs +++ b/src/mmap_xen.rs @@ -1077,26 +1077,18 @@ mod tests { range.mmap_flags = 16; let r = MmapXen::new(&range); - assert_eq!( - format!("{:?}", r.unwrap_err()), - format!("MmapFlags({})", range.mmap_flags), - ); + assert!(matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == range.mmap_flags)); range.mmap_flags = MmapXenFlags::FOREIGN.bits() | MmapXenFlags::GRANT.bits(); let r = MmapXen::new(&range); - assert_eq!( - format!("{:?}", r.unwrap_err()), - format!("MmapFlags({:x})", MmapXenFlags::ALL.bits()), + assert!( + matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == MmapXenFlags::ALL.bits()) ); range.mmap_flags = MmapXenFlags::FOREIGN.bits() | MmapXenFlags::NO_ADVANCE_MAP.bits(); let r = MmapXen::new(&range); - assert_eq!( - format!("{:?}", r.unwrap_err()), - format!( - "MmapFlags({:x})", - MmapXenFlags::NO_ADVANCE_MAP.bits() | MmapXenFlags::FOREIGN.bits(), - ), + assert!( + matches!(r.unwrap_err(), Error::MmapFlags(flags) if flags == MmapXenFlags::NO_ADVANCE_MAP.bits() | MmapXenFlags::FOREIGN.bits()) ); } @@ -1132,17 +1124,17 @@ mod tests { range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 0)); range.prot = None; let r = MmapXenForeign::new(&range); - assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError"); + assert!(matches!(r.unwrap_err(), Error::UnexpectedError)); let mut range = MmapRange::initialized(true); range.flags = None; let r = MmapXenForeign::new(&range); - assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError"); + assert!(matches!(r.unwrap_err(), Error::UnexpectedError)); let mut range = MmapRange::initialized(true); range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 1)); let r = MmapXenForeign::new(&range); - assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength"); + assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength)); let mut range = MmapRange::initialized(true); range.size = 0; @@ -1164,7 +1156,7 @@ mod tests { let mut range = MmapRange::initialized(true); range.prot = None; let r = MmapXenGrant::new(&range, MmapXenFlags::empty()); - assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError"); + assert!(matches!(r.unwrap_err(), Error::UnexpectedError)); let mut range = MmapRange::initialized(true); range.prot = None; @@ -1174,12 +1166,12 @@ mod tests { let mut range = MmapRange::initialized(true); range.flags = None; let r = MmapXenGrant::new(&range, MmapXenFlags::NO_ADVANCE_MAP); - assert_eq!(format!("{:?}", r.unwrap_err()), "UnexpectedError"); + assert!(matches!(r.unwrap_err(), Error::UnexpectedError)); let mut range = MmapRange::initialized(true); range.file_offset = Some(FileOffset::new(TempFile::new().unwrap().into_file(), 1)); let r = MmapXenGrant::new(&range, MmapXenFlags::NO_ADVANCE_MAP); - assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength"); + assert!(matches!(r.unwrap_err(), Error::InvalidOffsetLength)); let mut range = MmapRange::initialized(true); range.size = 0; diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index 486f5720..bd0b0f59 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -1488,58 +1488,6 @@ mod tests { slice.compute_end_offset(6, 0).unwrap_err(); } - #[test] - fn test_display_error() { - assert_eq!( - format!("{}", Error::OutOfBounds { addr: 0x10 }), - "address 0x10 is out of bounds" - ); - - assert_eq!( - format!( - "{}", - Error::Overflow { - base: 0x0, - offset: 0x10 - } - ), - "address 0x0 offset by 0x10 would overflow" - ); - - assert_eq!( - format!( - "{}", - Error::TooBig { - nelements: 100_000, - size: 1_000_000_000 - } - ), - "100000 elements of size 1000000000 would overflow a usize" - ); - - assert_eq!( - format!( - "{}", - Error::Misaligned { - addr: 0x4, - alignment: 8 - } - ), - "address 0x4 is not aligned to 8" - ); - - assert_eq!( - format!( - "{}", - Error::PartialBuffer { - expected: 100, - completed: 90 - } - ), - "only used 90 bytes in 100 long buffer" - ); - } - #[test] fn misaligned_ref() { let mut a = [0u8; 3];