filesys: a handle on an existing file is writable

src/filesys.rs

@@ -1006,7 +1006,7 @@ impl FilesysUnit {
                 if path.is_dir() {
                     return (DOSFALSE, ERROR_OBJECT_WRONG_TYPE);
                 }
-                match std::fs::File::open(&path) {
+                match open_existing(&path) {
                     Ok(f) => {
                         self.next_file_key += 1;
                         let key = self.next_file_key;
@@ -1054,7 +1054,7 @@ impl FilesysUnit {
                 if path.is_dir() {
                     return (DOSFALSE, ERROR_OBJECT_WRONG_TYPE);
                 }
-                match std::fs::File::open(&path) {
+                match open_existing(&path) {
                     Ok(f) => {
                         self.next_file_key += 1;
                         let key = self.next_file_key;
@@ -1983,6 +1983,17 @@ fn civil_from_days(z: i64) -> (i64, i64, i64) {
     (if m <= 2 { y + 1 } else { y }, m, d)
 }
 
+/// Open an existing file writable, the way AmigaDOS hands out a file handle;
+/// fall back to read-only if the host refuses, so a later write fails as it
+/// would on a protected Amiga file.
+fn open_existing(path: &Path) -> std::io::Result<std::fs::File> {
+    std::fs::OpenOptions::new()
+        .read(true)
+        .write(true)
+        .open(path)
+        .or_else(|_| std::fs::File::open(path))
+}
+
 /// Map a host I/O error onto the AmigaDOS error the guest expects, so a full
 /// disk says "disk full" rather than a generic failure.
 fn host_error(e: &std::io::Error) -> u32 {
@@ -2248,6 +2259,81 @@ mod tests {
         }
     }
 
+    /// A single mounted HOSTFS unit backed by a temp dir, over a flat guest
+    /// RAM, for driving `handle_packet` without a machine. Fixed guest
+    /// addresses for the objects a packet references; the temp dir is removed
+    /// on drop.
+    struct FsTester {
+        root: PathBuf,
+        board: FilesysBoard,
+        bus: RamBus,
+        guest_op: Option<GuestOp>,
+    }
+
+    impl FsTester {
+        const BOARD_BASE: u32 = 0x1_0000;
+        const PORT: u32 = 0x3000;
+        const PKT: u32 = 0x2000;
+        const DN: u32 = 0x1000;
+        const FH: u32 = 0x4000;
+        const NAME: u32 = 0x5000;
+        const BUF: u32 = 0x6000;
+
+        /// Mount `tag` and send the startup packet, so the unit is wired.
+        fn mounted(tag: &str) -> Self {
+            let root = std::env::temp_dir().join(format!("clfs-{tag}-{}", std::process::id()));
+            std::fs::create_dir_all(&root).unwrap();
+            let mut board = FilesysBoard::default();
+            board.set_mounts(vec![MountSpec {
+                path: root.clone(),
+                volume: "Test".into(),
+                boot_pri: -128,
+                readonly: false,
+            }]);
+            let mut fs = Self {
+                root,
+                board,
+                bus: RamBus(vec![0u8; 0x2_0000]),
+                guest_op: None,
+            };
+            fs.send(0, [0, 0, Self::DN >> 2]);
+            fs
+        }
+
+        /// Post one packet of type `ty` with three args; returns (Res1, Res2).
+        fn send(&mut self, ty: i32, args: [u32; 3]) -> (u32, u32) {
+            self.bus.write_long(Self::PKT + 8, ty as u32);
+            for (i, a) in args.iter().enumerate() {
+                self.bus.write_long(Self::PKT + 20 + 4 * i as u32, *a);
+            }
+            self.board.units[0].handle_packet(
+                &mut self.bus,
+                Self::BOARD_BASE,
+                Self::PORT,
+                Self::PKT,
+                &mut self.guest_op,
+            )
+        }
+
+        /// Lay a BSTR file name at NAME for the next packet to reference.
+        fn set_name(&mut self, s: &[u8]) {
+            self.bus.0[Self::NAME as usize] = s.len() as u8;
+            self.bus.0[Self::NAME as usize + 1..Self::NAME as usize + 1 + s.len()]
+                .copy_from_slice(s);
+        }
+
+        /// Write `bytes` into the shared buffer at BUF.
+        fn set_buf(&mut self, bytes: &[u8]) {
+            self.bus.0[Self::BUF as usize..Self::BUF as usize + bytes.len()].copy_from_slice(bytes);
+        }
+    }
+
+    impl Drop for FsTester {
+        fn drop(&mut self) {
+            let _ = std::fs::remove_dir_all(&self.root);
+        }
+    }
+
     /// The whole MMIO surface, end to end through [`ZorroDevice`]: the
     /// DIAG_DOORBELL captures the board base, the PORT write introduces the
     /// handler, and the packet doorbell handles a startup packet placed in
@@ -2529,114 +2615,87 @@ mod tests {
         std::fs::remove_dir_all(&root).unwrap();
     }
 
+    /// A handle on an existing file is writable, whichever way it was opened.
+    /// This is Workbench's Snapshot: icon.library takes a shared lock on the
+    /// .info, OpenFromLock()s it, reads the header, and writes it back through
+    /// the same handle. Opening the host file read-only failed that write.
+    #[test]
+    fn handles_on_existing_files_are_writable() {
+        let mut fs = FsTester::mounted("rw");
+        fs.set_buf(b"new");
+
+        // OpenFromLock() on a shared lock, then Open(MODE_OLDFILE), which is
+        // read/write on the Amiga as well.
+        for (file, from_lock) in [("a.info", true), ("b.info", false)] {
+            std::fs::write(fs.root.join(file), b"old").unwrap();
+            fs.set_name(file.as_bytes());
+            let open = if from_lock {
+                let (lock, _) =
+                    fs.send(ACTION_LOCATE_OBJECT, [0, FsTester::NAME >> 2, ACCESS_READ]);
+                fs.send(ACTION_FH_FROM_LOCK, [FsTester::FH >> 2, lock, 0])
+            } else {
+                fs.send(
+                    ACTION_FINDINPUT,
+                    [FsTester::FH >> 2, 0, FsTester::NAME >> 2],
+                )
+            };
+            assert_eq!(open, (DOSTRUE, 0), "open {file}");
+            let cookie = fs.bus.read_long(FsTester::FH + FILEHANDLE_ARG1);
+            assert_eq!(
+                fs.send(ACTION_WRITE, [cookie, FsTester::BUF, 3]),
+                (3, 0),
+                "write {file}"
+            );
+            fs.send(ACTION_END, [cookie, 0, 0]);
+            assert_eq!(std::fs::read(fs.root.join(file)).unwrap(), b"new");
+        }
+    }
+
     /// Seek offsets are signed LONGs: a backward relative seek (negative
     /// Arg2) must move the position, not zero-extend into a huge forward
     /// seek. This is iffparse's PopChunk pattern -- stream chunks with
     /// placeholder lengths, then Seek(-n, OFFSET_CURRENT/OFFSET_END) back
     /// and patch them -- which P96Prefs saves rely on.
     #[test]
-    fn seek_accepts_negative_offsets_like_iffparse() {
-        let root = std::env::temp_dir().join(format!("clfs-seek-{}", std::process::id()));
-        std::fs::create_dir_all(&root).unwrap();
+    fn seek_accepts_negative_offsets() {
+        let mut fs = FsTester::mounted("seek");
 
-        let mut hle = FilesysBoard::default();
-        hle.set_mounts(vec![MountSpec {
-            path: root.clone(),
-            volume: "Test".into(),
-            boot_pri: -128,
-            readonly: false,
-        }]);
-        let mut bus = RamBus(vec![0u8; 0x2_0000]);
-        let (board_base, port, pkt, dn, fh, name, buf) = (
-            0x1_0000u32,
-            0x3000u32,
-            0x2000u32,
-            0x1000u32,
-            0x4000u32,
-            0x5000u32,
-            0x6000u32,
-        );
-        let mut guest_op = None;
-        let send = |bus: &mut RamBus,
-                    unit: &mut FilesysUnit,
-                    guest_op: &mut Option<GuestOp>,
-                    ty: i32,
-                    args: [u32; 3]| {
-            bus.write_long(pkt + 8, ty as u32);
-            for (i, a) in args.iter().enumerate() {
-                bus.write_long(pkt + 20 + 4 * i as u32, *a);
-            }
-            unit.handle_packet(bus, board_base, port, pkt, guest_op)
-        };
-
-        // Startup packet wires the unit; then Open("iff.bin", MODE_NEWFILE).
-        let unit = &mut hle.units[0];
-        send(&mut bus, unit, &mut guest_op, 0, [0, 0, dn >> 2]);
-        bus.0[name as usize] = 7;
-        bus.0[name as usize + 1..name as usize + 8].copy_from_slice(b"iff.bin");
-        let (res1, res2) = send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_FINDOUTPUT,
-            [fh >> 2, 0, name >> 2],
+        // Open("iff.bin", MODE_NEWFILE).
+        fs.set_name(b"iff.bin");
+        assert_eq!(
+            fs.send(
+                ACTION_FINDOUTPUT,
+                [FsTester::FH >> 2, 0, FsTester::NAME >> 2]
+            ),
+            (DOSTRUE, 0)
         );
-        assert_eq!((res1, res2), (DOSTRUE, 0));
-        let cookie = bus.read_long(fh + FILEHANDLE_ARG1);
+        let cookie = fs.bus.read_long(FsTester::FH + FILEHANDLE_ARG1);
 
         // Stream 24 bytes with two 0xFFFFFFFF length placeholders.
-        bus.0[buf as usize..buf as usize + 24]
-            .copy_from_slice(b"FORM\xff\xff\xff\xffP96SANNO\xff\xff\xff\xffp96p");
-        let (res1, _) = send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_WRITE,
-            [cookie, buf, 24],
-        );
-        assert_eq!(res1, 24);
+        fs.set_buf(b"FORM\xff\xff\xff\xffP96SANNO\xff\xff\xff\xffp96p");
+        assert_eq!(fs.send(ACTION_WRITE, [cookie, FsTester::BUF, 24]).0, 24);
 
         // Patch the ANNO length: back 8 from the current position (24 -> 16).
-        let (res1, res2) = send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_SEEK,
-            [cookie, (-8i32) as u32, OFFSET_CURRENT as u32],
-        );
-        assert_eq!((res1, res2), (24, 0), "backward OFFSET_CURRENT seek failed");
-        bus.0[buf as usize..buf as usize + 4].copy_from_slice(&4u32.to_be_bytes());
-        send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_WRITE,
-            [cookie, buf, 4],
+        assert_eq!(
+            fs.send(ACTION_SEEK, [cookie, (-8i32) as u32, OFFSET_CURRENT as u32]),
+            (24, 0),
+            "backward OFFSET_CURRENT seek failed"
         );
+        fs.set_buf(&4u32.to_be_bytes());
+        fs.send(ACTION_WRITE, [cookie, FsTester::BUF, 4]);
 
         // Patch the FORM length: 20 back from the end (24 -> 4).
-        let (res1, res2) = send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_SEEK,
-            [cookie, (-20i32) as u32, OFFSET_END as u32],
-        );
-        assert_eq!((res1, res2), (20, 0), "backward OFFSET_END seek failed");
-        bus.0[buf as usize..buf as usize + 4].copy_from_slice(&16u32.to_be_bytes());
-        send(
-            &mut bus,
-            unit,
-            &mut guest_op,
-            ACTION_WRITE,
-            [cookie, buf, 4],
+        assert_eq!(
+            fs.send(ACTION_SEEK, [cookie, (-20i32) as u32, OFFSET_END as u32]),
+            (20, 0),
+            "backward OFFSET_END seek failed"
         );
+        fs.set_buf(&16u32.to_be_bytes());
+        fs.send(ACTION_WRITE, [cookie, FsTester::BUF, 4]);
 
-        send(&mut bus, unit, &mut guest_op, ACTION_END, [cookie, 0, 0]);
-        let on_disk = std::fs::read(root.join("iff.bin")).unwrap();
+        fs.send(ACTION_END, [cookie, 0, 0]);
+        let on_disk = std::fs::read(fs.root.join("iff.bin")).unwrap();
         assert_eq!(on_disk, b"FORM\x00\x00\x00\x10P96SANNO\x00\x00\x00\x04p96p");
-
-        std::fs::remove_dir_all(&root).unwrap();
     }
 
     /// Latin-1 <-> UTF-8 mapping matches amiberry: a guest name with high bytes