tests: switch to QueueT

Make all accesses to the underlying QueueState go through the with() function,
which will take care of locking when testing QueueSync.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Author: bonzini

crates/virtio-queue/src/generic.rs

@@ -5,8 +5,9 @@
 use std::num::Wrapping;
 use std::ops::DerefMut;
 
-use crate::{Error, Queue, QueueGuard, QueueState};
+use crate::{Error, Queue, QueueGuard, QueueState, QueueSync};
 use std::sync::atomic::Ordering;
+use std::sync::{Arc, Mutex, MutexGuard};
 use vm_memory::GuestAddressSpace;
 
 /// Lifetime-generic guard associated to a QueueStateT. In practice,
@@ -174,3 +175,32 @@ impl<M: GuestAddressSpace> QueueT<M> for Queue<M> {
         f(self.acquire())
     }
 }
+
+impl<'g, M: GuestAddressSpace> QueueStateGuard<'g> for QueueSync<M> {
+    type Out = MutexGuard<'g, QueueState>;
+}
+
+impl<M: GuestAddressSpace> QueueT<M> for QueueSync<M> {
+    type Guard = Self;
+
+    fn construct(mem: M, state: QueueState) -> Self {
+        QueueSync {
+            mem,
+            state: Arc::new(Mutex::new(state)),
+        }
+    }
+    fn with<
+        'a,
+        'g,
+        U,
+        F: FnOnce(QueueGuard<M::T, <Self::Guard as QueueStateGuard<'g>>::Out>) -> U,
+    >(
+        &'a mut self,
+        f: F,
+    ) -> U
+    where
+        'a: 'g,
+    {
+        f(self.lock())
+    }
+}

crates/virtio-queue/src/lib.rs

@@ -29,6 +29,7 @@ use std::mem::size_of;
 use std::num::Wrapping;
 use std::ops::{Deref, DerefMut};
 use std::sync::atomic::{fence, Ordering};
+use std::sync::{Arc, Mutex, MutexGuard};
 
 use log::error;
 use vm_memory::{
@@ -1028,9 +1029,37 @@ impl<M: GuestAddressSpace> Queue<M> {
     }
 }
 
+/// A convenient wrapper struct for a thread-safe virtio queue, with associated GuestMemory object.
+#[derive(Clone, Debug)]
+pub struct QueueSync<M: GuestAddressSpace> {
+    /// Guest memory object associated with the queue.
+    pub mem: M,
+    /// Virtio queue state.
+    pub state: Arc<Mutex<QueueState>>,
+}
+
+impl<M: GuestAddressSpace> QueueSync<M> {
+    /// Construct an empty virtio queue with the given `max_size`.
+    pub fn new(mem: M, max_size: u16) -> Self {
+        QueueSync {
+            mem,
+            state: Arc::new(Mutex::new(QueueState::new(max_size))),
+        }
+    }
+
+    /// Get an exclusive reference to the underlying `QueueState` object.
+    ///
+    /// Logically this method will acquire the underlying lock protecting the `QueueState`
+    /// object. The lock will be released when the returned object gets dropped.
+    pub fn lock(&self) -> QueueGuard<M::T, MutexGuard<QueueState>> {
+        QueueGuard::new(self.mem.memory(), self.state.lock().unwrap())
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
+    use generic::QueueT;
     use memoffset::offset_of;
     use mock::{DescriptorTable, MockSplitQueue};
 
@@ -1193,72 +1222,74 @@ mod tests {
         assert!(q.is_valid());
 
         // shouldn't be valid when not marked as ready
-        q.state.ready = false;
+        q.with(|mut qstate| qstate.ready = false);
         assert!(!q.is_valid());
-        q.state.ready = true;
+        q.with(|mut qstate| qstate.ready = true);
+
+        let max_size = q.with(|qstate| qstate.max_size);
 
         // or when size > max_size
-        q.state.size = q.state.max_size << 1;
+        q.with(|mut qstate| qstate.size = max_size << 1);
         assert!(!q.is_valid());
-        q.state.size = q.state.max_size;
+        q.with(|mut qstate| qstate.size = max_size);
 
         // or when size is 0
-        q.state.size = 0;
+        q.with(|mut qstate| qstate.size = 0);
         assert!(!q.is_valid());
-        q.state.size = q.state.max_size;
+        q.with(|mut qstate| qstate.size = max_size);
 
         // or when size is not a power of 2
-        q.state.size = 11;
+        q.with(|mut qstate| qstate.size = 11);
         assert!(!q.is_valid());
-        q.state.size = q.state.max_size;
+        q.with(|mut qstate| qstate.size = max_size);
 
         // or if the various addresses are off
 
-        q.state.desc_table = GuestAddress(0xffff_ffff);
+        q.with(|mut qstate| qstate.desc_table = GuestAddress(0xffff_ffff));
         assert!(!q.is_valid());
-        q.state.desc_table = GuestAddress(0x1001);
+        q.with(|mut qstate| qstate.desc_table = GuestAddress(0x1001));
         assert!(!q.is_valid());
-        q.state.desc_table = vq.desc_table_addr();
+        q.with(|mut qstate| qstate.desc_table = vq.desc_table_addr());
 
-        q.state.avail_ring = GuestAddress(0xffff_ffff);
+        q.with(|mut qstate| qstate.avail_ring = GuestAddress(0xffff_ffff));
         assert!(!q.is_valid());
-        q.state.avail_ring = GuestAddress(0x1001);
+        q.with(|mut qstate| qstate.avail_ring = GuestAddress(0x1001));
         assert!(!q.is_valid());
-        q.state.avail_ring = vq.avail_addr();
+        q.with(|mut qstate| qstate.avail_ring = vq.avail_addr());
 
-        q.state.used_ring = GuestAddress(0xffff_ffff);
+        q.with(|mut qstate| qstate.used_ring = GuestAddress(0xffff_ffff));
         assert!(!q.is_valid());
-        q.state.used_ring = GuestAddress(0x1001);
+        q.with(|mut qstate| qstate.used_ring = GuestAddress(0x1001));
         assert!(!q.is_valid());
-        q.state.used_ring = vq.used_addr();
+        q.with(|mut qstate| qstate.used_ring = vq.used_addr());
 
-        {
+        q.with(|mut qstate| {
             // an invalid queue should return an iterator with no next
-            q.state.ready = false;
-            let mut i = q.iter().unwrap();
+            qstate.ready = false;
+            let mut i = qstate.iter().unwrap();
             assert!(i.next().is_none());
-        }
+        });
 
-        q.state.ready = true;
+        q.with(|mut qstate| qstate.ready = true);
 
         // now let's create two simple descriptor chains
         // the chains are (0, 1) and (2, 3, 4)
-        {
-            for j in 0..5u16 {
-                let flags = match j {
-                    1 | 4 => 0,
-                    _ => VIRTQ_DESC_F_NEXT,
-                };
-
-                let desc = Descriptor::new((0x1000 * (j + 1)) as u64, 0x1000, flags, j + 1);
-                vq.desc_table().store(j, desc);
-            }
+        for j in 0..5u16 {
+            let flags = match j {
+                1 | 4 => 0,
+                _ => VIRTQ_DESC_F_NEXT,
+            };
 
-            vq.avail().ring().ref_at(0).store(0);
-            vq.avail().ring().ref_at(1).store(2);
-            vq.avail().idx().store(2);
+            let desc = Descriptor::new((0x1000 * (j + 1)) as u64, 0x1000, flags, j + 1);
+            vq.desc_table().store(j, desc);
+        }
+
+        vq.avail().ring().ref_at(0).store(0);
+        vq.avail().ring().ref_at(1).store(2);
+        vq.avail().idx().store(2);
 
-            let mut i = q.iter().unwrap();
+        q.with(|mut qstate| {
+            let mut i = qstate.iter().unwrap();
 
             {
                 let mut c = i.next().unwrap();
@@ -1285,13 +1316,13 @@ mod tests {
             {
                 assert!(i.next().is_none());
                 i.go_to_previous_position();
-                let mut c = q.iter().unwrap().next().unwrap();
+                let mut c = qstate.iter().unwrap().next().unwrap();
                 c.next().unwrap();
                 c.next().unwrap();
                 c.next().unwrap();
                 assert!(c.next().is_none());
             }
-        }
+        })
     }
 
     #[test]
@@ -1321,39 +1352,41 @@ mod tests {
         vq.avail().ring().ref_at(2).store(5);
         vq.avail().idx().store(3);
 
-        let mut i = q.iter().unwrap();
+        q.with(|mut qstate| {
+            let mut i = qstate.iter().unwrap();
 
-        {
-            let c = i.next().unwrap();
-            assert_eq!(c.head_index(), 0);
-
-            let mut iter = c;
-            assert!(iter.next().is_some());
-            assert!(iter.next().is_some());
-            assert!(iter.next().is_none());
-            assert!(iter.next().is_none());
-        }
+            {
+                let c = i.next().unwrap();
+                assert_eq!(c.head_index(), 0);
 
-        {
-            let c = i.next().unwrap();
-            assert_eq!(c.head_index(), 2);
-
-            let mut iter = c.writable();
-            assert!(iter.next().is_some());
-            assert!(iter.next().is_some());
-            assert!(iter.next().is_none());
-            assert!(iter.next().is_none());
-        }
+                let mut iter = c;
+                assert!(iter.next().is_some());
+                assert!(iter.next().is_some());
+                assert!(iter.next().is_none());
+                assert!(iter.next().is_none());
+            }
 
-        {
-            let c = i.next().unwrap();
-            assert_eq!(c.head_index(), 5);
+            {
+                let c = i.next().unwrap();
+                assert_eq!(c.head_index(), 2);
 
-            let mut iter = c.readable();
-            assert!(iter.next().is_some());
-            assert!(iter.next().is_none());
-            assert!(iter.next().is_none());
-        }
+                let mut iter = c.writable();
+                assert!(iter.next().is_some());
+                assert!(iter.next().is_some());
+                assert!(iter.next().is_none());
+                assert!(iter.next().is_none());
+            }
+
+            {
+                let c = i.next().unwrap();
+                assert_eq!(c.head_index(), 5);
+
+                let mut iter = c.readable();
+                assert!(iter.next().is_some());
+                assert!(iter.next().is_none());
+                assert!(iter.next().is_none());
+            }
+        })
     }
 
     #[test]
@@ -1371,7 +1404,7 @@ mod tests {
 
         // should be ok
         q.add_used(1, 0x1000).unwrap();
-        assert_eq!(q.state.next_used, Wrapping(1));
+        q.with(|qstate| assert_eq!(qstate.next_used, Wrapping(1)));
         assert_eq!(vq.used().idx().load(), 1);
 
         let x = vq.used().ring().ref_at(0).load();
@@ -1399,11 +1432,13 @@ mod tests {
         let vq = MockSplitQueue::new(m, 16);
 
         let mut q: Queue<_> = vq.as_queue(m);
-        q.state.size = 8;
-        q.state.ready = true;
-        q.state.reset();
-        assert_eq!(q.state.size, 16);
-        assert_eq!(q.state.ready, false);
+        q.with(|mut qstate| {
+            qstate.size = 8;
+            qstate.ready = true;
+            qstate.reset();
+            assert_eq!(qstate.size, 16);
+            assert_eq!(qstate.ready, false);
+        })
     }
 
     #[test]
@@ -1417,21 +1452,24 @@ mod tests {
 
         // It should always return true when EVENT_IDX isn't enabled.
         for i in 0..qsize {
-            q.state.next_used = Wrapping(i);
+            q.with(|mut qstate| qstate.next_used = Wrapping(i));
             assert_eq!(q.needs_notification().unwrap(), true);
         }
 
         m.write_obj::<u16>(4, avail_addr.unchecked_add(4 + qsize as u64 * 2))
             .unwrap();
-        q.state.set_event_idx(true);
+        q.with(|mut qstate| qstate.set_event_idx(true));
 
         // Incrementing up to this value causes an `u16` to wrap back to 0.
         let wrap = u32::from(u16::MAX) + 1;
 
         for i in 0..wrap + 12 {
-            q.state.next_used = Wrapping(i as u16);
-            // Let's test wrapping around the maximum index value as well.
-            let expected = i == 5 || i == (5 + wrap) || q.state.signalled_used.is_none();
+            let expected = q.with(|mut qstate| {
+                qstate.next_used = Wrapping(i as u16);
+                // Let's test wrapping around the maximum index value as well.
+                i == 5 || i == (5 + wrap) || qstate.signalled_used.is_none()
+            });
+
             assert_eq!(q.needs_notification().unwrap(), expected);
         }
 
@@ -1445,9 +1483,9 @@ mod tests {
             .unwrap();
 
         assert_eq!(q.needs_notification().unwrap(), false);
-        q.state.next_used = Wrapping(15);
+        q.with(|mut qstate| qstate.next_used = Wrapping(15));
         assert_eq!(q.needs_notification().unwrap(), false);
-        q.state.next_used = Wrapping(0);
+        q.with(|mut qstate| qstate.next_used = Wrapping(0));
         assert_eq!(q.needs_notification().unwrap(), true);
         assert_eq!(q.needs_notification().unwrap(), false);
     }
@@ -1460,7 +1498,7 @@ mod tests {
         let mut q: Queue<_> = vq.as_queue(m);
         let used_addr = vq.used_addr();
 
-        assert_eq!(q.state.event_idx_enabled, false);
+        q.with(|qstate| assert_eq!(qstate.event_idx_enabled, false));
 
         q.enable_notification().unwrap();
         let v = m.read_obj::<u16>(used_addr).unwrap();
@@ -1479,13 +1517,13 @@ mod tests {
         m.write_obj::<u16>(2, avail_addr.unchecked_add(2)).unwrap();
 
         assert_eq!(q.enable_notification().unwrap(), true);
-        q.state.next_avail = Wrapping(2);
+        q.with(|mut qstate| qstate.next_avail = Wrapping(2));
         assert_eq!(q.enable_notification().unwrap(), false);
 
         m.write_obj::<u16>(8, avail_addr.unchecked_add(2)).unwrap();
 
         assert_eq!(q.enable_notification().unwrap(), true);
-        q.state.next_avail = Wrapping(8);
+        q.with(|mut qstate| qstate.next_avail = Wrapping(8));
         assert_eq!(q.enable_notification().unwrap(), false);
     }
 }