Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partialeq #206

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Partialeq #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6bd80f
[guest_memory]: Implement PartialEq for Error enum
AlexandruCihodaru Oct 10, 2022
4775dd7
[volatile_memory]: Implement PartialEq for Error
AlexandruCihodaru Oct 10, 2022
80f6dc8
[mmap_unix]: Implement PartialEq for Error enum
AlexandruCihodaru Oct 10, 2022
0ac2b35
[mmap]: Derive PartialEq for Error enum
AlexandruCihodaru Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/guest_memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,33 @@ pub enum Error {
HostAddressNotAvailable,
}

impl PartialEq for Error {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason I don't particularly like these kinds of implementations for PartialEq is that it will silently fail when you add a new error type if you don't explicitly add it in the below match as well. I would propose we add a test that just fails when a new variant is added and has a comment something like: "If this failed it means you added a new error type. Make sure you also add that to the PartialEq implementation. Once you've done that feel free to fix this test". This will serve as a warning when adding new types. I am also open to other ideas as this is rather dummy 😆

(Error::InvalidGuestAddress(left), Error::InvalidGuestAddress(right)) => left == right,
(Error::InvalidBackendAddress, Error::InvalidBackendAddress) => true,
(Error::HostAddressNotAvailable, Error::HostAddressNotAvailable) => true,
(
Error::PartialBuffer {
expected: left_expected,
completed: left_completed,
},
Error::PartialBuffer {
expected: right_expected,
completed: right_completed,
},
) => left_expected == right_expected && left_completed == right_completed,
(Error::IOError(left), Error::IOError(right)) => {
// error.kind should be enough to assert equallity because each error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is true, it does not mean that kind can be used to compare errors. The reason why io::Error does not have PartialEq implemented is because it cannot be implemented truthfully. The first example that comes to mind is NotFound. A not found error thrown for file1 is not equal to an error thrown for file2. This is one of the reasons why we cannot actually compare the io::Error like that because it has other metadata that is specific to the error type. We can say instead that we are willing to accept the loss of information in comparing errors because vm-memory errors are widely used in other crates as well and it is very limiting to not have PartialEq implemented.

// has the kind field set and the OS error numbers can be converted
// to ErrorKind through `sys::decode_error_kind`.
left.kind() == right.kind()
}
_ => false,
}
}
}

impl From<volatile_memory::Error> for Error {
fn from(e: volatile_memory::Error) -> Self {
match e {
Expand Down
156 changes: 53 additions & 103 deletions src/mmap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub(crate) trait AsSlice {
}

/// Errors that can occur when creating a memory map.
#[derive(Debug)]
#[derive(Debug, PartialEq)]
pub enum Error {
/// Adding the guest base address to the length of the underlying mapping resulted
/// in an overflow.
Expand Down Expand Up @@ -775,43 +775,21 @@ mod tests {
#[test]
fn test_no_memory_region() {
let regions_summary = [];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::NoMemoryRegion)
new_guest_memory_mmap_from_arc_regions(&regions_summary).unwrap_err(),
Error::NoMemoryRegion
);
}

Expand All @@ -820,41 +798,20 @@ mod tests {
let regions_summary = [(GuestAddress(0), 100_usize), (GuestAddress(99), 100_usize)];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::MemoryRegionOverlap)
new_guest_memory_mmap_from_arc_regions(&regions_summary).unwrap_err(),
Error::MemoryRegionOverlap
);
}

Expand All @@ -863,41 +820,20 @@ mod tests {
let regions_summary = [(GuestAddress(100), 100_usize), (GuestAddress(0), 100_usize)];

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap(&regions_summary).err().unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
new_guest_memory_mmap(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_with_files(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
new_guest_memory_mmap_with_files(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
new_guest_memory_mmap_from_regions(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
);

assert_eq!(
format!(
"{:?}",
new_guest_memory_mmap_from_arc_regions(&regions_summary)
.err()
.unwrap()
),
format!("{:?}", Error::UnsortedMemoryRegions)
new_guest_memory_mmap_from_arc_regions(&regions_summary).unwrap_err(),
Error::UnsortedMemoryRegions
);
}

Expand Down Expand Up @@ -1054,7 +990,10 @@ mod tests {

let guest_mem_list = vec![guest_mem, guest_mem_backed_by_file];
for guest_mem in guest_mem_list.iter() {
assert!(guest_mem.get_host_address(GuestAddress(0x600)).is_err());
assert_eq!(
guest_mem.get_host_address(GuestAddress(0x600)).unwrap_err(),
guest_memory::Error::InvalidGuestAddress(GuestAddress(0x600))
);
let ptr0 = guest_mem.get_host_address(GuestAddress(0x800)).unwrap();
let ptr1 = guest_mem.get_host_address(GuestAddress(0xa00)).unwrap();
assert_eq!(
Expand Down Expand Up @@ -1122,18 +1061,16 @@ mod tests {
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,)
gm.write_obj(val1, bad_addr).unwrap_err(),
guest_memory::Error::InvalidGuestAddress(bad_addr)
);
assert_eq!(
format!("{:?}", gm.write_obj(val1, bad_addr2).err().unwrap()),
format!(
"PartialBuffer {{ expected: {:?}, completed: {:?} }}",
mem::size_of::<u64>(),
max_addr.checked_offset_from(bad_addr2).unwrap()
)
gm.write_obj(val1, bad_addr2).unwrap_err(),
guest_memory::Error::PartialBuffer {
expected: mem::size_of::<u64>(),
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();
let num1: u64 = gm.read_obj(GuestAddress(0x500)).unwrap();
Expand Down Expand Up @@ -1435,7 +1372,11 @@ mod tests {
// Error case when slice_size is beyond the boundary.
let slice_addr = MemoryRegionAddress(0x300);
let slice_size = 0x200;
assert!(region.get_slice(slice_addr, slice_size).is_err());

assert_eq!(
region.get_slice(slice_addr, slice_size).unwrap_err(),
guest_memory::Error::InvalidBackendAddress
);
}

#[test]
Expand Down Expand Up @@ -1483,9 +1424,18 @@ mod tests {
.is_empty());

// Error cases, wrong size or base address.
assert!(guest_mem.get_slice(GuestAddress(0), 0x500).is_err());
assert!(guest_mem.get_slice(GuestAddress(0x600), 0x100).is_err());
assert!(guest_mem.get_slice(GuestAddress(0xc00), 0x100).is_err());
assert_eq!(
guest_mem.get_slice(GuestAddress(0), 0x500).unwrap_err(),
guest_memory::Error::InvalidBackendAddress
);
assert_eq!(
guest_mem.get_slice(GuestAddress(0x600), 0x100).unwrap_err(),
guest_memory::Error::InvalidGuestAddress(GuestAddress(0x600))
);
assert_eq!(
guest_mem.get_slice(GuestAddress(0xc00), 0x100).unwrap_err(),
guest_memory::Error::InvalidGuestAddress(GuestAddress(0xc00))
);
}

#[test]
Expand Down
52 changes: 34 additions & 18 deletions src/mmap_unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,24 @@ pub enum Error {
SeekStart(io::Error),
}

impl PartialEq for Error {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
// error.kind should be enough to assert equallity because each error
// has the kind field set and the OS error numbers can be converted
// to ErrorKind through `sys::decode_error_kind`.
(Error::Mmap(left), Error::Mmap(right))
| (Error::SeekEnd(left), Error::SeekEnd(right))
| (Error::SeekStart(left), Error::SeekStart(right)) => left.kind() == right.kind(),
(Error::InvalidOffsetLength, Error::InvalidOffsetLength) => true,
(Error::InvalidPointer, Error::InvalidPointer) => true,
(Error::MapFixed, Error::MapFixed) => true,
(Error::MappingOverlap, Error::MappingOverlap) => true,
(Error::MappingPastEof, Error::MappingPastEof) => true,
_ => false,
}
}
}
impl fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter) -> fmt::Result {
match self {
Expand Down Expand Up @@ -467,20 +485,12 @@ mod tests {

type MmapRegion = super::MmapRegion<()>;

// Adding a helper method to extract the errno within an Error::Mmap(e), or return a
// distinctive value when the error is represented by another variant.
impl Error {
pub fn raw_os_error(&self) -> i32 {
match self {
Error::Mmap(e) => e.raw_os_error().unwrap(),
_ => std::i32::MIN,
}
}
}

#[test]
fn test_mmap_region_new() {
assert!(MmapRegion::new(0).is_err());
assert_eq!(
MmapRegion::new(0).unwrap_err(),
Error::Mmap(std::io::Error::from_raw_os_error(libc::EINVAL))
);

let size = 4096;

Expand All @@ -496,7 +506,10 @@ mod tests {

#[test]
fn test_mmap_region_set_hugetlbfs() {
assert!(MmapRegion::new(0).is_err());
assert_eq!(
MmapRegion::new(0).unwrap_err(),
Error::Mmap(std::io::Error::from_raw_os_error(libc::EINVAL))
);

let size = 4096;

Expand Down Expand Up @@ -567,7 +580,7 @@ mod tests {
prot,
flags,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength");
assert_eq!(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(
Expand All @@ -576,7 +589,7 @@ mod tests {
prot,
flags,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "MappingPastEof");
assert_eq!(r.unwrap_err(), Error::MappingPastEof);

// MAP_FIXED was specified among the flags.
let r = MmapRegion::build(
Expand All @@ -585,7 +598,7 @@ mod tests {
prot,
flags | libc::MAP_FIXED,
);
assert_eq!(format!("{:?}", r.unwrap_err()), "MapFixed");
assert_eq!(r.unwrap_err(), Error::MapFixed);

// Let's resize the file.
assert_eq!(unsafe { libc::ftruncate(a.as_raw_fd(), 1024 * 10) }, 0);
Expand All @@ -597,7 +610,10 @@ mod tests {
prot,
flags,
);
assert_eq!(r.unwrap_err().raw_os_error(), libc::EINVAL);
assert_eq!(
r.unwrap_err(),
Error::Mmap(std::io::Error::from_raw_os_error(libc::EINVAL))
);

// The build should be successful now.
let r =
Expand Down Expand Up @@ -629,7 +645,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_eq!(r.unwrap_err(), Error::InvalidPointer);

let r = unsafe { MmapRegion::build_raw(addr as *mut u8, size, prot, flags).unwrap() };

Expand Down
Loading