Add Mac sanity checks when Apple quirks mode is enabled #303
Conversation
The main purpose of this code is to emit warnings when Mac quirks are
turned on and the image(s) are disks.
The warnings are emitted if any of the following checks fail:
- The disk image contain Apple Magic signature
- The HFS block size is correct
- The Mac SCSI driver is smaller than or equal to the max size
- The MAC SCSI driver is smaller than or equal to the image size
- The Lido driver is not installed
The original implementation put the sanity checks under platform code.
Since none of the code is platform specific, it has been moved
to the root src directory so all build targets can take advantage
of the sanity checks.
Initial code authored by Eric Helgeson and taken from:
BlueSCSI/BlueSCSI-v2@cc1ff5f
BlueSCSI/BlueSCSI-v2@d00ebe0
Co-authored-by: Eric Helgeson <[email protected]>
src/QuirksCheck.cpp
Outdated
|
|
||
| void quirksCheck(image_config_t *img) | ||
| { | ||
| macQuirksSanityCheck(img); |
There was a problem hiding this comment.
I think it would make sense to have if (img->quirks == S2S_CFG_QUIRKS_APPLE) { macQuirksSanityCheck(img); } here, instead of checking it on line 96.
That way there won't be any spurious debug messages when the quirks aren't enabled for the device.
src/QuirksCheck.cpp
Outdated
| dbgmsg("---- Skipping Mac sanity check due to DisableMacSanityCheck"); | ||
| return; | ||
| } | ||
| if(img->file.isRom() || img->file.isRaw()) |
There was a problem hiding this comment.
ImageBackingStore constructor converts contiguous files to raw mappings. So this check will skip most image files.
Is there a particular reason to skip the check for raw mappings?
There was a problem hiding this comment.
I think BlueSCSI wanted to skip raw pass through and neither of us understood what m_israw actually means. Getting rid of it.
There was a problem hiding this comment.
I've got it seeking and reading on SD_SECTOR_SIZE blocks, that means we can actually do a sanity check for ROM images too?
src/QuirksCheck.cpp
Outdated
|
|
||
| // Check for Apple Magic | ||
| img->file.seek(0); | ||
| img->file.read(tmp, 2); |
There was a problem hiding this comment.
I think the reason why raw mappings are excluded in the macQuirksSanityCheck() is these unaligned accesses, which won't work because raw access must be in whole SD blocks. This ends up excluding any continuous image file.
This could be modified to be uint8_t tmp[SD_SECTOR_SIZE]; img->file.seek(0); img.file.read(tmp, SD_SECTOR_SIZE); and then comparing against specific bytes in the tmp array.
src/ImageBackingStore.cpp
Outdated
| * Portions - Copyright (C) 2023 Eric Helgeson | ||
| * ZuluSCSI™ - Copyright (c) 2022-2023 Rabbit Hole Computing™ |
There was a problem hiding this comment.
Order of copyright notices seems wrong, most of the code in this file is from Rabbit Hole Computing.
|
Thanks for the feedback @PetteriAimonen, I'll implement the changes. |
Addressed recommendation from code review in the pull request: #303 The main issue was the ImageBackingStore store driver seeks and reads weren't being accessed by SD card blocks of 512 bytes. After remedying the issue, both raw and rom mode can go through the sanity checks.
|
@PetteriAimonen I think I addressed all the issues. I also removed the isRom() check because all reads and seeks are on SD_SECTOR_SIZE boundaries. |
|
@morio I noticed that the CI builds for ZuluSCSI V1.x are failing: |
ZuluSCSI's based on the GD32F205 were failing to build because of a missing assert.h include in src/QuirksCheck.cpp. Added the include and the builds were successful.
aperezbios
changed the title
The main purpose of this code is to emit warnings when Mac quirks are turned on and the image(s) are disks.
The warnings are emitted if any of the following checks fail:
- The disk image contain Apple Magic signature
- The HFS block size is correct
- The Mac SCSI driver is smaller than or equal to the max size
- The MAC SCSI driver is smaller than or equal to the image size
- The Lido driver is not installed
The original implementation put the sanity checks under platform code. Since none of the code is platform specific, it has been moved to the root src directory so all build targets can take advantage of the sanity checks.
Initial code authored by Eric Helgeson and taken from: BlueSCSI/BlueSCSI-v2@cc1ff5f BlueSCSI/BlueSCSI-v2@d00ebe0