Skip to content

Commit

Permalink
unittest: net: eliminate mocking
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@roypat
roypat committed Sep 16, 2024
1 parent 1d0a725 commit 271aed8
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 84 deletions.
45 changes: 10 additions & 35 deletions src/vmm/src/devices/virtio/net/device.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<usize> {
self.tap.read(&mut self.rx_frame_buf)
}

#[cfg(not(test))]
fn write_tap(tap: &mut Tap, buf: &IoVecBuffer) -> std::io::Result<usize> {
tap.write_iovec(buf)
}
Expand Down Expand Up @@ -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};

Expand All @@ -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;
Expand All @@ -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<usize> {
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<usize> {
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];
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 0 additions & 9 deletions src/vmm/src/devices/virtio/net/tap.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
})
}

Expand Down Expand Up @@ -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(),
Expand Down
41 changes: 1 addition & 40 deletions src/vmm/src/devices/virtio/net/test_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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<u8> {
self.net().tap.mocks.set_read_tap(ReadTapMock::TapFrame);
let used_idx = self.rxq.used.idx.get();

// Inject frame to tap and run epoll.
Expand Down

0 comments on commit 271aed8

Please sign in to comment.