From 271aed89d796b73e7a73e5688fbec92229498db4 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 15:14:43 +0100 Subject: [PATCH] unittest: net: eliminate mocking There was a very elaborate mocking setup in the net device code involving `cfg(not(test))`, but the only point of it all was to be able to test the scenario where `tap.write_iovec` or `tap.read` return an error. We do not need mocking to achieve this though: We can simply close the tap fd. Thus remove all the mocking, and update the negative tests to close the fd. I had half a mind to just delete the tests, as currently we are not sensibly handling errors in device emulation anyway, so the assertions these tests make aren't exactly useful. However, once we implement device reset, this will probably change, so let's keep the tests. Signed-off-by: Patrick Roy --- src/vmm/src/devices/virtio/net/device.rs | 45 +++++--------------- src/vmm/src/devices/virtio/net/tap.rs | 9 ---- src/vmm/src/devices/virtio/net/test_utils.rs | 41 +----------------- 3 files changed, 11 insertions(+), 84 deletions(-) diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 5bdbd43b89c..6fb91f9c8a5 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -5,7 +5,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the THIRD-PARTY file. -#[cfg(not(test))] use std::io::Read; use std::mem; use std::net::Ipv4Addr; @@ -716,12 +715,10 @@ impl Net { self.tx_rate_limiter.update_buckets(tx_bytes, tx_ops); } - #[cfg(not(test))] fn read_tap(&mut self) -> std::io::Result { self.tap.read(&mut self.rx_frame_buf) } - #[cfg(not(test))] fn write_tap(tap: &mut Tap, buf: &IoVecBuffer) -> std::io::Result { tap.write_iovec(buf) } @@ -932,11 +929,11 @@ impl VirtioDevice for Net { #[cfg(test)] #[macro_use] pub mod tests { - use std::io::Read; use std::net::Ipv4Addr; + use std::os::fd::AsRawFd; use std::str::FromStr; use std::time::Duration; - use std::{io, mem, thread}; + use std::{mem, thread}; use utils::net::mac::{MacAddr, MAC_ADDR_LEN}; @@ -949,8 +946,8 @@ pub mod tests { }; use crate::devices::virtio::net::test_utils::test::TestHelper; use crate::devices::virtio::net::test_utils::{ - default_net, if_index, inject_tap_tx_frame, set_mac, NetEvent, NetQueue, ReadTapMock, - TapTrafficSimulator, WriteTapMock, + default_net, if_index, inject_tap_tx_frame, set_mac, NetEvent, NetQueue, + TapTrafficSimulator, }; use crate::devices::virtio::net::NET_QUEUE_SIZES; use crate::devices::virtio::queue::VIRTQ_DESC_F_WRITE; @@ -961,28 +958,6 @@ pub mod tests { use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenBucket, TokenType}; use crate::vstate::memory::{Address, GuestMemory}; - impl Net { - pub(crate) fn read_tap(&mut self) -> io::Result { - match &self.tap.mocks.read_tap { - ReadTapMock::Failure => Err(io::Error::new( - io::ErrorKind::Other, - "Read tap synthetically failed.", - )), - ReadTapMock::TapFrame => self.tap.read(&mut self.rx_frame_buf), - } - } - - pub(crate) fn write_tap(tap: &mut Tap, buf: &IoVecBuffer) -> io::Result { - match tap.mocks.write_tap { - WriteTapMock::Success => tap.write_iovec(buf), - WriteTapMock::Failure => Err(io::Error::new( - io::ErrorKind::Other, - "Write tap mock failure.", - )), - } - } - } - #[test] fn test_vnet_helpers() { let mut frame_buf = vec![42u8; vnet_hdr_len() - 1]; @@ -1212,7 +1187,6 @@ pub mod tests { fn test_rx_retry() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame); // Add invalid descriptor chain - read only descriptor. th.add_desc_chain( @@ -1263,7 +1237,6 @@ pub mod tests { fn test_rx_complex_desc_chain() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame); // Create a valid Rx avail descriptor chain with multiple descriptors. th.add_desc_chain( @@ -1302,7 +1275,6 @@ pub mod tests { fn test_rx_multiple_frames() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame); // Create 2 valid Rx avail descriptor chains. Each one has enough space to fit the // following 2 frames. But only 1 frame has to be written to each chain. @@ -1525,7 +1497,9 @@ pub mod tests { fn test_tx_tap_failure() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_write_tap(WriteTapMock::Failure); + // force the next write to the tap to return an error by simply closing the fd + // SAFETY: its a valid fd + unsafe { libc::close(th.net.lock().unwrap().tap.as_raw_fd()) }; let desc_list = [(0, 1000, 0)]; th.add_desc_chain(NetQueue::Tx, 0, &desc_list); @@ -1725,7 +1699,9 @@ pub mod tests { fn test_read_tap_fail_event_handler() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_read_tap(ReadTapMock::Failure); + // force the next write to the tap to return an error by simply closing the fd + // SAFETY: its a valid fd + unsafe { libc::close(th.net.lock().unwrap().tap.as_raw_fd()) }; // The RX queue is empty and rx_deffered_frame is set. th.net().rx_deferred_frame = true; @@ -1753,7 +1729,6 @@ pub mod tests { fn test_deferred_frame() { let mut th = TestHelper::get_default(); th.activate_net(); - th.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame); let rx_packets_count = th.net().metrics.rx_packets_count.count(); let _ = inject_tap_tx_frame(&th.net(), 1000); diff --git a/src/vmm/src/devices/virtio/net/tap.rs b/src/vmm/src/devices/virtio/net/tap.rs index a6ab82996ce..d6ab7d3f840 100644 --- a/src/vmm/src/devices/virtio/net/tap.rs +++ b/src/vmm/src/devices/virtio/net/tap.rs @@ -16,8 +16,6 @@ use utils::{ioctl_ioc_nr, ioctl_iow_nr}; use crate::devices::virtio::iovec::IoVecBuffer; use crate::devices::virtio::net::gen; -#[cfg(test)] -use crate::devices::virtio::net::test_utils::Mocks; // As defined in the Linux UAPI: // https://elixir.bootlin.com/linux/v4.17/source/include/uapi/linux/if.h#L33 @@ -53,9 +51,6 @@ ioctl_iow_nr!(TUNSETVNETHDRSZ, TUNTAP, 216, ::std::os::raw::c_int); pub struct Tap { tap_file: File, pub(crate) if_name: [u8; IFACE_NAME_MAX_LEN], - - #[cfg(test)] - pub(crate) mocks: Mocks, } // Returns a byte vector representing the contents of a null terminated C string which @@ -149,9 +144,6 @@ impl Tap { tap_file: tuntap, // SAFETY: Safe since only the name is accessed, and it's cloned out. if_name: unsafe { ifreq.ifr_ifrn.ifrn_name }, - - #[cfg(test)] - mocks: Mocks::default(), }) } @@ -278,7 +270,6 @@ pub mod tests { let faulty_tap = Tap { tap_file: unsafe { File::from_raw_fd(-2) }, if_name: [0x01; 16], - mocks: Default::default(), }; assert_eq!( faulty_tap.set_vnet_hdr_size(16).unwrap_err().to_string(), diff --git a/src/vmm/src/devices/virtio/net/test_utils.rs b/src/vmm/src/devices/virtio/net/test_utils.rs index 8285619cea9..57d8780fcd1 100644 --- a/src/vmm/src/devices/virtio/net/test_utils.rs +++ b/src/vmm/src/devices/virtio/net/test_utils.rs @@ -76,44 +76,6 @@ pub fn default_net_no_mmds() -> Net { net } -#[derive(Debug)] -pub enum ReadTapMock { - Failure, - TapFrame, -} - -#[derive(Debug)] -pub enum WriteTapMock { - Failure, - Success, -} - -// Used to simulate tap read and write fails in tests. -#[derive(Debug)] -pub struct Mocks { - pub(crate) read_tap: ReadTapMock, - pub(crate) write_tap: WriteTapMock, -} - -impl Mocks { - pub fn set_read_tap(&mut self, read_tap: ReadTapMock) { - self.read_tap = read_tap; - } - - pub fn set_write_tap(&mut self, write_tap: WriteTapMock) { - self.write_tap = write_tap; - } -} - -impl Default for Mocks { - fn default() -> Mocks { - Mocks { - read_tap: ReadTapMock::TapFrame, - write_tap: WriteTapMock::Success, - } - } -} - #[derive(Debug)] pub enum NetQueue { Rx, @@ -347,7 +309,7 @@ pub mod test { use crate::devices::virtio::net::device::vnet_hdr_len; use crate::devices::virtio::net::gen::ETH_HLEN; use crate::devices::virtio::net::test_utils::{ - assign_queues, default_net, inject_tap_tx_frame, NetEvent, NetQueue, ReadTapMock, + assign_queues, default_net, inject_tap_tx_frame, NetEvent, NetQueue, }; use crate::devices::virtio::net::{Net, MAX_BUFFER_SIZE, RX_INDEX, TX_INDEX}; use crate::devices::virtio::queue::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; @@ -476,7 +438,6 @@ pub mod test { /// Generate a tap frame of `frame_len` and check that it is deferred pub fn check_rx_deferred_frame(&mut self, frame_len: usize) -> Vec { - self.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame); let used_idx = self.rxq.used.idx.get(); // Inject frame to tap and run epoll.