Fix some bugs in hole seeking on Windows

src/lib.rs

@@ -947,7 +947,7 @@ fn punch_value(opts: PunchValueOptions) -> Result<()> {
 }
 
 pub fn check_hole(file: &mut File, offset: u64, length: u64) -> Result<()> {
-    match seek_hole_whence(file, offset as i64, seekhole::Data)? {
+    match seek_hole_whence(file, offset, seekhole::Data)? {
         // Data starts after the hole we just punched.
         Some(seek_offset) if seek_offset >= offset + length => Ok(()),
         // There's no data after the hole we just punched.

src/main.rs

@@ -8,9 +8,9 @@ use itertools::Itertools;
 use log::info;
 use possum::sys::punchfile::punchfile;
 use possum::sys::seekhole::{file_regions, Region, RegionType};
+use possum::sys::SparseFile;
 use possum::tx::ReadTransactionRef;
 use possum::{ceil_multiple, check_hole, Handle, NonzeroValueLocation, WalkEntry};
-use possum::sys::SparseFile;
 
 #[derive(clap::Subcommand)]
 enum Commands {
@@ -149,7 +149,8 @@ fn main() -> anyhow::Result<()> {
                         {
                             // Read access might be required to query allocated ranges on Windows.
                             let mut file = std::fs::OpenOptions::new()
-                                .write(true).read(true)
+                                .write(true)
+                                .read(true)
                                 .open(&values_file_entry.path)?;
                             file.set_sparse(true)?;
                             // Make sure nobody could be writing to the file. It should be possible

src/sys/punchfile/mod.rs

@@ -15,3 +15,21 @@ cfg_if! {
 }
 
 use super::*;
+
+#[cfg(test)]
+mod tests {
+    use self::test;
+    use super::*;
+    use tempfile::NamedTempFile;
+
+    #[test]
+    fn hole_punching() -> anyhow::Result<()> {
+        let mut temp_file = NamedTempFile::new()?;
+        let file = temp_file.as_file_mut();
+        file.set_sparse(true)?;
+        file.set_len(2)?;
+        punchfile(file, 0, 1)?;
+        check_hole(file, 0, 1)?;
+        Ok(())
+    }
+}

src/sys/seekhole/mod.rs

@@ -81,7 +81,7 @@ impl Iterator for Iter<'_> {
         let mut whence = first_whence;
         // This only runs twice. Once with each whence, starting with the one we didn't try last.
         loop {
-            match seek_hole_whence(self.file, self.offset as i64, whence) {
+            match seek_hole_whence(self.file, self.offset, whence) {
                 Ok(Some(offset)) if offset != self.offset => {
                     let region = Region {
                         region_type: !whence,
@@ -146,27 +146,35 @@ mod tests {
         if min_hole_size <= 1 {
             min_hole_size = 2;
         }
-        let mut temp_file = write_random_tempfile(min_hole_size)?;
-        let regions = file_regions(temp_file.as_file_mut())?;
+        let mut temp_file = write_random_tempfile(2 * min_hole_size)?;
+        let file_ref = temp_file.as_file_mut();
+        let regions = file_regions(file_ref)?;
         assert_eq!(
             regions,
             vec![Region {
                 region_type: Data,
                 start: 0,
-                end: min_hole_size
+                end: 2 * min_hole_size
             }]
         );
-        temp_file.as_file().set_sparse(true)?;
-        punchfile(temp_file.as_file(), 0, min_hole_size as i64)?;
-        temp_file.seek(Start(0))?;
-        let regions: Vec<_> = file_regions(temp_file.as_file_mut())?;
+        file_ref.set_sparse(true)?;
+        punchfile(file_ref, 0, min_hole_size as i64)?;
+        file_ref.seek(Start(0))?;
+        let regions: Vec<_> = file_regions(file_ref)?;
         assert_eq!(
             regions,
-            vec![Region {
-                region_type: Hole,
-                start: 0,
-                end: min_hole_size
-            }]
+            vec![
+                Region {
+                    region_type: Hole,
+                    start: 0,
+                    end: min_hole_size
+                },
+                Region {
+                    region_type: Data,
+                    start: min_hole_size,
+                    end: 2 * min_hole_size,
+                }
+            ]
         );
         Ok(())
     }

src/sys/seekhole/windows.rs

@@ -1,10 +1,11 @@
 use super::*;
 
 pub fn seek_hole_whence(
-    file: &mut File,
-    offset: i64,
+    file: &File,
+    offset: u64,
     whence: RegionType,
 ) -> io::Result<Option<RegionOffset>> {
+    let offset = offset.try_into().unwrap();
     let mut output = Vec::with_capacity(1);
     let file_offset = match whence {
         Hole => offset,
@@ -19,16 +20,24 @@ pub fn seek_hole_whence(
         }],
         &mut output,
     )?;
+    dbg!(&output);
+    // match returns Result<Some<i64>> because FILE_ALLOCATED_RANGE_BUFFER uses i64.
     match whence {
         Hole => match output[..] {
-            [next_range, ..] => Ok(Some(
-                (next_range.FileOffset + next_range.Length) as RegionOffset,
-            )),
-            [] => Ok(Some(offset as RegionOffset)),
+            [next_range, ..] => {
+                assert!(next_range.FileOffset >= offset);
+                Ok(Some(if next_range.FileOffset == offset {
+                    next_range.FileOffset + next_range.Length
+                } else {
+                    offset
+                }))
+            }
+            [] => Ok(Some(offset)),
         },
         Data => match output[..] {
-            [next_range, ..] => Ok(Some(next_range.FileOffset as RegionOffset)),
+            [next_range, ..] => Ok(Some(next_range.FileOffset)),
             [] => Ok(None),
         },
     }
+    .map(|ok| ok.map(|some| some.try_into().unwrap()))
 }

src/sys/windows.rs

@@ -82,7 +82,7 @@ pub(crate) fn device_io_control<I: ?Sized, O: ?Sized>(
     };
     let out_buffer = output.map(|o| o as *mut O as _);
     let overlapped = overlapped.map(|some| some as *mut _);
-    unsafe {
+    if let Err(err) = unsafe {
         DeviceIoControl(
             handle,
             control_code,
@@ -93,6 +93,12 @@ pub(crate) fn device_io_control<I: ?Sized, O: ?Sized>(
             lp_bytes_returned,
             overlapped,
         )
-    }?;
+    } {
+        // No need to flag this to the caller unless they are doing an operation that wouldn't
+        // otherwise try again with a new starting point.
+        if err.code() != ERROR_MORE_DATA.into() {
+            return Err(err.into());
+        }
+    }
     Ok(bytes_returned)
 }

tests/simple_tests.rs

@@ -310,26 +310,6 @@ fn torrent_storage_inner(opts: TorrentStorageOpts) -> Result<()> {
     assert_eq!(completed_hash, piece_data_hash);
     let handle_walk_entries = handle.walk_dir()?;
     let _counts = count_by_entry_types(&handle_walk_entries);
-    // ValuesFile count calculation might need changing if this doesn't hold.
-    assert_eq!(block_size as u64 % handle.block_size(), 0);
-    // This might all be reusable as a Handle current disk usage calculation.
-    let mut values_file_total_len = 0;
-    for entry in &handle_walk_entries {
-        if entry.entry_type != ValuesFile {
-            continue;
-        }
-        values_file_total_len += match possum::sys::path_disk_allocation(&entry.path) {
-            Ok(size) => size,
-            Err(err) if err.kind() == ErrorKind::NotFound => 0,
-            Err(err) => return Err(err).context(entry.path.display().to_string()),
-        };
-    }
-    if false {
-        dbg!(values_file_total_len);
-        assert!([0, 1]
-            .map(|extra_blocks| extra_blocks * block_size + 2 * piece_size)
-            .contains(&(values_file_total_len as usize)));
-    }
     assert_eq!(handle.list_items("a".as_bytes())?.len(), 0);
     assert_eq!(handle.list_items("c".as_bytes())?.len(), 1);
     let offsets_starting_with_1 = offsets_starting_with(block_offset_iter, "1").count();
@@ -532,7 +512,7 @@ where
 #[test]
 fn test_writeback_mmap() -> anyhow::Result<()> {
     let mut file = OpenOptions::new()
-        .append(true)
+        .write(true)
         .create(true)
         .open("writeback")?;
     file.set_len(0x1000)?;