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

Jump to bottom

Remove code duplication in tests #140

Closed
wants to merge 4 commits into from
Closed

Remove code duplication in tests #140

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59b3f0f
Remove code duplication in tests
DeliaPavel Feb 28, 2022
f9f74d6
Merge branch 'main' into main
DeliaPavel Mar 1, 2022
9365ec5
Merge branch 'rust-vmm:main' into main
DeliaPavel Mar 16, 2022
23f42fa
Merge branch 'rust-vmm:main' into main
DeliaPavel Mar 22, 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
72 changes: 45 additions & 27 deletions crates/virtio-queue/src/queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl<M: GuestAddressSpace> Queue<M, QueueState> {
}

#[cfg(test)]
mod tests {
pub mod tests {
use super::*;
use crate::defs::{
DEFAULT_AVAIL_RING_ADDR, DEFAULT_DESC_TABLE_ADDR, DEFAULT_USED_RING_ADDR,
Expand Down Expand Up @@ -639,18 +639,15 @@ mod tests {
assert_eq!(q.next_used(), 7);
}

#[test]
fn test_invalid_avail_idx() {
// This is a negative test for the following MUST from the spec: `A driver MUST NOT
// decrement the available idx on a virtqueue (ie. there is no way to “unexpose” buffers).`.
// We validate that for this misconfiguration, the device does not panic.
let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
let vq = MockSplitQueue::new(m, 16);

let mut q = vq.create_queue(m);

// q is currently valid.
assert!(q.is_valid());
/// This is a test case that checks that an invalid avail_idx is not triggering a panic in
/// the queue implementation.
pub fn check_invalid_avail_idx(
state: &mut QueueState,
mem: &GuestMemoryMmap,
vq: &MockSplitQueue<GuestMemoryMmap>,
) {
// self is currently valid.
assert!(state.is_valid(mem));

// The chains are (0, 1), (2, 3, 4), (5, 6).
for i in 0..7 {
Expand All @@ -668,14 +665,15 @@ mod tests {
vq.avail().ring().ref_at(2).store(u16::to_le(5));
// Let the device know it can consume chains with the index < 2.
vq.avail().idx().store(u16::to_le(3));

// No descriptor chains are consumed at this point.
assert_eq!(q.next_avail(), 0);
assert_eq!(q.next_used(), 0);
assert_eq!(state.next_avail(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this refactoring is that you are no longer calling these methods from the Queue/QueueGuard implementation, but always from the QueueState one. Because of this, having two separate tests doesn't actually make much sense anymore. I'll think about what we can do regarding this.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't find any solution to this without some serious refactoring. One way of achieving this is to implement the same trait on both QueueGuard and Queue, and the test case receive as a parameter a parametrized Q where Q implements the common trait. This is extending far beyond the scope of the opened issue. The code in Queue and QueueGuard is duplicated as well, but I think that needs to be handled as a separate issue

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find any solution to this without some serious refactoring. One way of achieving this is to implement the same trait on both QueueGuard and Queue, and the test case receive as a parameter a parametrized Q where Q implements the common trait. This is extending far beyond the scope of the opened issue. The code in Queue and QueueGuard is duplicated as well, but I think that needs to be handled as a separate issue

I agree, so should we drop this patch for now?

assert_eq!(state.next_used(), 0);

loop {
q.disable_notification().unwrap();
state.disable_notification(mem.deref()).unwrap();

while let Some(chain) = q.iter().unwrap().next() {
while let Some(chain) = state.iter(mem).unwrap().next() {
// Process the descriptor chain, and then add entries to the
// used ring.
let head_index = chain.head_index();
Expand All @@ -685,33 +683,53 @@ mod tests {
desc_len += d.len();
}
});
q.add_used(head_index, desc_len).unwrap();
state.add_used(mem.deref(), head_index, desc_len).unwrap();
}
if !q.enable_notification().unwrap() {
if !state.enable_notification(mem.deref()).unwrap() {
break;
}
}

// The next chain that can be consumed should have index 3.
assert_eq!(q.next_avail(), 3);
assert_eq!(q.avail_idx(Ordering::Acquire).unwrap(), Wrapping(3));
assert_eq!(q.next_used(), 3);
assert_eq!(q.used_idx(Ordering::Acquire).unwrap(), Wrapping(3));
assert!(q.lock().ready());
assert_eq!(state.next_avail(), 3);
assert_eq!(
state.avail_idx(mem.deref(), Ordering::Acquire).unwrap(),
Wrapping(3)
);
assert_eq!(state.next_used(), 3);
assert_eq!(
state.used_idx(mem.deref(), Ordering::Acquire).unwrap(),
Wrapping(3)
);
assert!(state.lock().ready());

// Decrement `idx` which should be forbidden. We don't enforce this thing, but we should
// test that we don't panic in case the driver decrements it.
vq.avail().idx().store(u16::to_le(1));

loop {
q.disable_notification().unwrap();
state.disable_notification(mem.deref()).unwrap();

while let Some(_chain) = q.iter().unwrap().next() {
while let Some(_chain) = state.iter(mem).unwrap().next() {
// In a real use case, we would do something with the chain here.
}

if !q.enable_notification().unwrap() {
if !state.enable_notification(mem.deref()).unwrap() {
break;
}
}
}

#[test]
fn test_invalid_avail_idx() {
// This is a negative test for the following MUST from the spec: `A driver MUST NOT
// decrement the available idx on a virtqueue (ie. there is no way to “unexpose” buffers).`.
// We validate that for this misconfiguration, the device does not panic.
let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
let vq = &MockSplitQueue::new(m, 16);

let mut q = vq.create_queue(m);

check_invalid_avail_idx(&mut q.state, m, vq);
}
}
73 changes: 4 additions & 69 deletions crates/virtio-queue/src/queue_guard.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,86 +200,21 @@ where

#[cfg(test)]
mod tests {
use super::*;
use crate::defs::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE};
use crate::mock::MockSplitQueue;
use crate::Descriptor;

use crate::queue::tests::check_invalid_avail_idx;
use vm_memory::{GuestAddress, GuestMemoryMmap};

#[test]
fn test_queue_guard_object() {
fn test_invalid_avail_idx() {
let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
andreeaflorescu marked this conversation as resolved.
Show resolved Hide resolved
let vq = MockSplitQueue::new(m, 0x100);
let vq = &MockSplitQueue::new(m, 0x100);
let mut q = vq.create_queue(m);
let mut g = q.lock_with_memory();

// g is currently valid.
assert!(g.is_valid());
assert!(g.ready());
assert_eq!(g.max_size(), 0x100);
g.set_size(16);

// The chains are (0, 1), (2, 3, 4), (5, 6).
for i in 0..7 {
let flags = match i {
1 | 4 | 6 => 0,
_ => VIRTQ_DESC_F_NEXT,
};

let desc = Descriptor::new((0x1000 * (i + 1)) as u64, 0x1000, flags, i + 1);
vq.desc_table().store(i, desc);
}

vq.avail().ring().ref_at(0).store(u16::to_le(0));
vq.avail().ring().ref_at(1).store(u16::to_le(2));
vq.avail().ring().ref_at(2).store(u16::to_le(5));
// Let the device know it can consume chains with the index < 2.
vq.avail().idx().store(u16::to_le(3));
// No descriptor chains are consumed at this point.
assert_eq!(g.next_avail(), 0);
assert_eq!(g.next_used(), 0);

loop {
g.disable_notification().unwrap();

while let Some(chain) = g.iter().unwrap().next() {
// Process the descriptor chain, and then add entries to the
// used ring.
let head_index = chain.head_index();
let mut desc_len = 0;
chain.for_each(|d| {
if d.flags() & VIRTQ_DESC_F_WRITE == VIRTQ_DESC_F_WRITE {
desc_len += d.len();
}
});
g.add_used(head_index, desc_len).unwrap();
}
if !g.enable_notification().unwrap() {
break;
}
}
// The next chain that can be consumed should have index 3.
assert_eq!(g.next_avail(), 3);
assert_eq!(g.avail_idx(Ordering::Acquire).unwrap(), Wrapping(3));
assert_eq!(g.next_used(), 3);
assert_eq!(g.used_idx(Ordering::Acquire).unwrap(), Wrapping(3));
assert!(g.ready());

// Decrement `idx` which should be forbidden. We don't enforce this thing, but we should
// test that we don't panic in case the driver decrements it.
vq.avail().idx().store(1);

loop {
g.disable_notification().unwrap();

while let Some(_chain) = g.iter().unwrap().next() {
// In a real use case, we would do something with the chain here.
}

if !g.enable_notification().unwrap() {
break;
}
}
check_invalid_avail_idx(g.state, m, vq);
}
}