Clear BPB dirty flags before creating FileSystem.

Author: reitermarkus

src/boot_sector.rs

@@ -334,6 +334,10 @@ impl BiosParameterBlock {
         FsStatusFlags::decode(self.reserved_1)
     }
 
+    pub(crate) fn set_status_flags(&mut self, status_flags: FsStatusFlags) {
+        self.reserved_1 = status_flags.encode();
+    }
+
     pub(crate) fn is_fat32(&self) -> bool {
         // because this field must be zero on FAT32, and
         // because it must be non-zero on FAT12/FAT16,

src/fs.rs

@@ -101,7 +101,7 @@ impl FsStatusFlags {
         self.io_error
     }
 
-    const fn encode(self) -> u8 {
+    pub(crate) const fn encode(self) -> u8 {
         let mut res = 0_u8;
         if self.dirty {
             res |= 1;
@@ -386,7 +386,7 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
         debug_assert!(disk.seek(SeekFrom::Current(0))? == 0);
 
         // read boot sector
-        let bpb = {
+        let mut bpb = {
             let boot = BootSector::deserialize(&mut disk)?;
             boot.validate()?;
             boot.bpb
@@ -409,6 +409,15 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
 
         // if dirty flag is set completly ignore free_cluster_count in FSInfo
         if bpb_status_flags.dirty() {
+            if options.ignore_dirty_flag {
+                warn!("BPB is dirty, clearing dirty flag.");
+                bpb_status_flags.dirty = false;
+                write_bpb_status_flags(&mut disk, fat_type, bpb_status_flags)?;
+                bpb.set_status_flags(bpb_status_flags);
+            } else {
+                return Err(Error::DirtyFileSystem);
+            }
+
             fs_info.free_cluster_count = None;
         }
 
@@ -427,17 +436,6 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
             current_status_flags: Cell::new(bpb_status_flags),
         };
 
-        if bpb_status_flags.dirty() {
-            if fs.options.ignore_dirty_flag {
-                warn!("BPB is dirty, clearing dirty flag.");
-                bpb_status_flags.dirty = false;
-                fs.set_bpb_status_flags(bpb_status_flags)?;
-                fs.current_status_flags.get_mut().dirty = false;
-            } else {
-                return Err(Error::DirtyFileSystem)
-            }
-        }
-
         let mut fat_status_flags = fs.read_fat_status_flags()?;
         if fat_status_flags.dirty() {
             if fs.options.ignore_dirty_flag {
@@ -554,7 +552,7 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
     ///
     /// `Error::Io` will be returned if the underlying storage object returned an I/O error.
     pub fn read_status_flags(&self) -> Result<FsStatusFlags, Error<IO::Error>> {
-        let bpb_status = self.bpb.status_flags();
+        let bpb_status = self.current_status_flags.get();
         let fat_status = self.read_fat_status_flags()?;
         Ok(FsStatusFlags {
             dirty: bpb_status.dirty || fat_status.dirty,
@@ -631,28 +629,9 @@ impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
 
         status_flags.dirty = dirty;
 
-        self.set_bpb_status_flags(status_flags)?;
-
-        self.current_status_flags.set(status_flags);
-
-        Ok(())
-    }
-
-    #[inline]
-    fn set_bpb_status_flags(&self, status_flags: FsStatusFlags) -> Result<(), Error<IO::Error>> {
-        let encoded = status_flags.encode();
-
-        // Note: only one field is written to avoid rewriting entire boot-sector which could be dangerous
-        // Compute reserver_1 field offset and write new flags
-        let offset = if self.fat_type() == FatType::Fat32 {
-            0x041
-        } else {
-            0x025
-        };
-
         let mut disk = self.disk.borrow_mut();
-        disk.seek(io::SeekFrom::Start(offset))?;
-        disk.write_u8(encoded)?;
+        write_bpb_status_flags(&mut *disk, self.fat_type(), status_flags)?;
+        self.current_status_flags.set(status_flags);
 
         Ok(())
     }
@@ -794,6 +773,23 @@ impl<IO: ReadWriteSeek, TP, OCC> Clone for FsIoAdapter<'_, IO, TP, OCC> {
     }
 }
 
+fn write_bpb_status_flags<IO: Seek + Write>(
+    disk: &mut IO,
+    fat_type: FatType,
+    status_flags: FsStatusFlags,
+) -> Result<(), Error<IO::Error>> {
+    let encoded = status_flags.encode();
+
+    // Note: only one field is written to avoid rewriting entire boot-sector which could be dangerous
+    // Compute reserver_1 field offset and write new flags
+    let offset = if fat_type == FatType::Fat32 { 0x041 } else { 0x025 };
+
+    disk.seek(io::SeekFrom::Start(offset))?;
+    disk.write_u8(encoded)?;
+
+    Ok(())
+}
+
 fn fat_slice<B: BorrowMut<S>, E, S: ReadWriteSeek>(io: B, bpb: &BiosParameterBlock) -> DiskSlice<B, E, S> {
     let sectors_per_fat = bpb.sectors_per_fat();
     let mirroring_enabled = bpb.mirroring_enabled();

tests/write.rs

@@ -4,7 +4,7 @@ use std::io::prelude::*;
 use std::mem;
 use std::str;
 
-use fatfs::{DefaultTimeProvider, FsOptions, LossyOemCpConverter, StdIoWrapper};
+use fatfs::{DefaultTimeProvider, Error, FsOptions, LossyOemCpConverter, StdIoWrapper};
 use fscommon::BufStream;
 
 const FAT12_IMG: &str = "fat12.img";
@@ -27,16 +27,18 @@ fn call_with_tmp_img<F: Fn(&str) -> ()>(f: F, filename: &str, test_seq: u32) {
     fs::remove_file(tmp_path).unwrap();
 }
 
-fn open_filesystem_rw(tmp_path: &str) -> FileSystem {
+fn open_filesystem_rw(tmp_path: &str, ignore_dirty_flag: bool) -> Result<FileSystem, Error<io::Error>> {
     let file = fs::OpenOptions::new().read(true).write(true).open(&tmp_path).unwrap();
     let buf_file = BufStream::new(file);
-    let options = FsOptions::new().update_accessed_date(true);
-    FileSystem::new(buf_file, options).unwrap()
+    let options = FsOptions::new()
+        .update_accessed_date(true)
+        .ignore_dirty_flag(ignore_dirty_flag);
+    FileSystem::new(buf_file, options)
 }
 
 fn call_with_fs<F: Fn(FileSystem) -> ()>(f: F, filename: &str, test_seq: u32) {
     let callback = |tmp_path: &str| {
-        let fs = open_filesystem_rw(tmp_path);
+        let fs = open_filesystem_rw(tmp_path, false).unwrap();
         f(fs);
     };
     call_with_tmp_img(&callback, filename, test_seq);
@@ -338,22 +340,23 @@ fn test_rename_file_fat32() {
 
 fn test_dirty_flag(tmp_path: &str) {
     // Open filesystem, make change, and forget it - should become dirty
-    let fs = open_filesystem_rw(tmp_path);
+    let fs = open_filesystem_rw(tmp_path, false).unwrap();
     let status_flags = fs.read_status_flags().unwrap();
     assert_eq!(status_flags.dirty(), false);
     assert_eq!(status_flags.io_error(), false);
     fs.root_dir().create_file("abc.txt").unwrap();
     mem::forget(fs);
     // Check if volume is dirty now
-    let fs = open_filesystem_rw(tmp_path);
-    let status_flags = fs.read_status_flags().unwrap();
-    assert_eq!(status_flags.dirty(), true);
-    assert_eq!(status_flags.io_error(), false);
-    fs.unmount().unwrap();
+    let fs = open_filesystem_rw(tmp_path, false);
+    assert!(matches!(fs, Err(Error::DirtyFileSystem)));
     // Make sure remounting does not clear the dirty flag
-    let fs = open_filesystem_rw(tmp_path);
+    // Check if volume is dirty now
+    let fs = open_filesystem_rw(tmp_path, false);
+    assert!(matches!(fs, Err(Error::DirtyFileSystem)));
+    // Make sure clearing the dirty flag allows mounting to succeed
+    let fs = open_filesystem_rw(tmp_path, true).unwrap();
     let status_flags = fs.read_status_flags().unwrap();
-    assert_eq!(status_flags.dirty(), true);
+    assert_eq!(status_flags.dirty(), false);
     assert_eq!(status_flags.io_error(), false);
 }