Fable says -:
Verdict: request changes. The fixes are real and mostly correct, but one new path can overwrite FAT entry 0, the sector-size check needs a decision from you, and the commit that claims to stop destructive follow-ups misses three of them. This is a static read of the PR head (0ae8e9cd52); I did not build or run it.
Must fix
- OpSetFileSize() can write to FAT entry 0 (ops.c:1048-1052). If the entry's first cluster is at or above eoc_mark - 7, the chain walk is skipped and last stays 0. Growing the file then calls SET_NEXT_CLUSTER(sb, 0, added), and the rollback path writes the bogus value back to entry 0 as well. This is reachable on FAT12/16 entries with a non-zero high cluster word, since FIRST_FILE_CLUSTER does not mask by FAT type. Rejecting a first >= 2 that is not a valid data cluster with ERROR_NOT_A_DOS_DISK closes it.
- Destructive follow-ups the PR leaves unchecked. Commit 930920cf2f is titled as propagating failures before destructive follow-up, but these still ignore errors:
- OpRenameFile() (ops.c:648, ops.c:671): UpdateDirEntry(&dde) is unchecked. If it fails, the new entry keeps file_size = 0 and the original is then deleted. The delete's own result is ignored too.
- Truncate on open (ops.c:313-324): unchecked GetDirEntry/UpdateDirEntry, then the cluster chain is freed regardless.
- OpWrite() (ops.c:945-954): the size and first-cluster update is unchecked.
Needs your decision
- Sector-size check rejects the SDHC whole-card node (volume.c:134). sdcard_bus.c:430-431 declares that node with 1024-byte blocks and two sectors per block, while the BPB says 512. An unpartitioned FAT card on that node would now fail with ERROR_NOT_A_DOS_DISK.
- Partitions are unaffected; partition.library takes the size from the drive geometry.
- The old behaviour was already wrong there: total_sectors was counted in 1024-byte units, so only the first half of the volume was reachable.
- Scaling first_device_sector and total_sectors when the block size is a multiple of the sector size would keep that case working.
- I did not confirm that the FAT handler is actually started on that node.
Looks correct
- Short-transfer check (disk.c:233): I checked ata, scsi, ahci, nvme, virtio, sdcard, USB mass storage, both trackdisk drivers, hostdisk, fdsk, ramdrive and diskimage in your local checkout (older than the PR base). All set io_Actual to bytes transferred on success. Out-of-tree and native m68k drivers remain a risk.
- DoDiskInsert(): fixes a NULL dereference when pool or name allocation fails, and clears glob->sb when ReadFATSuper() fails after setting it. That second one was a dangling pointer on the validator's ERROR_NOT_A_DOS_DISK exit.
- SetVolumeName(): no longer writes an uninitialised name into the boot block when AllocDirEntry() fails, and no longer leaks the boot buffer.
- OpSetFileSize() otherwise: now updates gl->size and resets the handle, which the old code never did, and writes the entry little-endian.
- DeleteDirEntry() reorder, WriteFileChunk() allocate-before-link, .info suffix bound: all fine.
Minor
- OpCreateDir() still has an unchecked GetDirEntry(&dh, 0, &de) at ops.c:725; the same call in OpDeleteFile() was fixed.
- In SetVolumeName(), a failed boot-block write now returns an error after the root entry was already renamed, leaving the in-memory name stale.
- The new chain walks treat a failed FAT read (returned as 0) as ERROR_NOT_A_DOS_DISK or end of chain; sb->fat_io_error is available to tell them apart.
- The rewrite strips every D(bug()) line and comment from OpSetFileSize() and DeleteDirEntry(), unlike the rest of the handler.
- All six commits have a subject line only, and the 65-case harness is not in the tree, so none of the claimed coverage can be re-run.
- Codacy reports action_required on the head commit; I did not look at what it flagged.