Review feedback:

* Don't promote as many things to `pub`, and make it explicit when that happens. * .div_ceil rather than doing it manually * Arbitrary fields in enums. * cargo xtask fmt fix.
microsoft · Jan 7, 2025 · bcf4792 · bcf4792
1 parent b6eb881
commit bcf4792
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 21 deletions.
diff --git a/vm/devices/storage/storvsp/Cargo.toml b/vm/devices/storage/storvsp/Cargo.toml
@@ -12,6 +12,9 @@ ioperf = ["dep:disklayer_ram"]
 # Enable generating arbitrary values of types useful for fuzzing.
 arbitrary = ["dep:arbitrary"]
 
+# Expose some implementation details publicly, used for fuzzing.
+fuzz_helpers = []
+
 [dependencies]
 arbitrary = { workspace = true, optional = true, features = ["derive"] }
 

diff --git a/vm/devices/storage/storvsp/fuzz/Cargo.toml b/vm/devices/storage/storvsp/fuzz/Cargo.toml
@@ -15,7 +15,7 @@ guestmem.workspace = true
 pal_async.workspace = true
 scsi_defs = {workspace = true, features = ["arbitrary"]}
 scsidisk.workspace = true
-storvsp = {workspace = true, features = ["arbitrary"]}
+storvsp = {workspace = true, features = ["arbitrary", "fuzz_helpers"]}
 storvsp_resources = {workspace = true, features = ["arbitrary"]}
 vmbus_async.workspace = true
 vmbus_channel.workspace = true

diff --git a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs
@@ -27,11 +27,10 @@ use zerocopy::AsBytes;
 use zerocopy::FromZeroes;
 
 #[derive(Arbitrary)]
-pub enum StorvspFuzzAction {
-    SendDataPacket,
+enum StorvspFuzzAction {
     SendReadWritePacket,
-    SendRawPacket,
-    ReadCompletion
+    SendRawPacket(FuzzOutgoingPacketType),
+    ReadCompletion,
 }
 
 #[derive(Arbitrary)]
@@ -61,9 +60,7 @@ async fn send_gpa_direct_packet(
     let start_page: u64 = gpa_start / PAGE_SIZE as u64;
     let end_page = start_page
         .checked_add(byte_len.try_into()?)
-        .and_then(|v| v.checked_add(PAGE_SIZE as u64))
-        .and_then(|v| v.checked_sub(1))
-        .and_then(|v| v.checked_div(PAGE_SIZE as u64))
+        .map(|v| v.div_ceil(PAGE_SIZE as u64))
         .ok_or(arbitrary::Error::IncorrectFormat)?;
 
     let gpns: Vec<u64> = (start_page..end_page).collect();
@@ -126,7 +123,6 @@ async fn send_arbitrary_readwrite_packet(
 
     scsi_req.payload[0..10].copy_from_slice(cdb.as_bytes());
 
-    // send the gpa packet
     send_gpa_direct_packet(
         guest,
         &[packet.as_bytes(), scsi_req.as_bytes()],
@@ -172,26 +168,24 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> {
         while !u.is_empty() {
             let action = u.arbitrary::<StorvspFuzzAction>()?;
             match action {
-                StorvspFuzzAction::SendDataPacket => {
-                    let packet = u.arbitrary::<protocol::Packet>()?;
-                    let _ = guest.send_data_packet_sync(&[packet.as_bytes()]).await;
-                }
                 StorvspFuzzAction::SendReadWritePacket => {
                     send_arbitrary_readwrite_packet(u, &mut guest).await?;
                 }
-                StorvspFuzzAction::SendRawPacket => {
-                    let packet_type = u.arbitrary()?;
+                StorvspFuzzAction::SendRawPacket(packet_type) => {
                     match packet_type {
                         FuzzOutgoingPacketType::AnyOutgoingPacket => {
                             let packet_types = [
                                 OutgoingPacketType::InBandNoCompletion,
                                 OutgoingPacketType::InBandWithCompletion,
                                 OutgoingPacketType::Completion,
                             ];
+                            let payload = u.arbitrary::<protocol::Packet>()?;
+                            // TODO: [use-arbitrary-input] (send a byte blob of arbitrary length rather
+                            // than a fixed-size arbitrary packet)
                             let packet = OutgoingPacket {
                                 transaction_id: u.arbitrary()?,
                                 packet_type: *u.choose(&packet_types)?,
-                                payload: &[&[0u8; 0]], // TODO: [use-arbitrary-input]
+                                payload: &[payload.as_bytes()], // TODO: [use-arbitrary-input]
                             };
 
                             guest.queue.split().1.write(packet).await?;

diff --git a/vm/devices/storage/storvsp/src/lib.rs b/vm/devices/storage/storvsp/src/lib.rs
@@ -6,10 +6,18 @@
 #[cfg(feature = "ioperf")]
 pub mod ioperf;
 
+#[cfg(feature = "fuzz_helpers")]
 pub mod protocol;
+#[cfg(feature = "fuzz_helpers")]
+pub mod test_helpers;
+
+#[cfg(not(feature = "fuzz_helpers"))]
+mod protocol;
+#[cfg(not(feature = "fuzz_helpers"))]
+mod test_helpers;
+
 pub mod resolver;
 mod save_restore;
-pub mod test_helpers;
 
 use crate::ring::gparange::GpnList;
 use crate::ring::gparange::MultiPagedRangeBuf;
@@ -274,7 +282,7 @@ impl Future for ScsiRequest {
 }
 
 #[derive(Debug, Error)]
-pub enum WorkerError {
+enum WorkerError {
     #[error("packet error")]
     PacketError(#[source] PacketError),
     #[error("queue error")]
@@ -284,7 +292,7 @@ pub enum WorkerError {
 }
 
 #[derive(Debug, Error)]
-pub enum PacketError {
+enum PacketError {
     #[error("Not transactional")]
     NotTransactional,
     #[error("Unrecognized operation {0:?}")]
@@ -1544,7 +1552,7 @@ impl ScsiControllerDisk {
     }
 }
 
-pub struct ScsiControllerState {
+struct ScsiControllerState {
     disks: RwLock<HashMap<ScsiPath, ScsiControllerDisk>>,
     rescan_notification_source: Mutex<Vec<futures::channel::mpsc::Sender<()>>>,
 }

diff --git a/vm/devices/storage/storvsp/src/test_helpers.rs b/vm/devices/storage/storvsp/src/test_helpers.rs
@@ -43,7 +43,7 @@ pub struct TestWorker {
 }
 
 impl TestWorker {
-    pub async fn teardown(self) -> Result<(), WorkerError> {
+    pub(crate) async fn teardown(self) -> Result<(), WorkerError> {
         self.task.await
     }