Procházet zdrojové kódy

Make code review changes

Addressed recommendation from code review in the pull request:
https://github.com/ZuluSCSI/ZuluSCSI-firmware/pull/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.
Morio před 2 roky
rodič
revize
1ff6c8f57f
2 změnil soubory, kde provedl 37 přidání a 40 odebrání
  1. 1 1
      src/ImageBackingStore.cpp
  2. 36 39
      src/QuirksCheck.cpp

+ 1 - 1
src/ImageBackingStore.cpp

@@ -1,6 +1,6 @@
 /**
- * Portions - Copyright (C) 2023 Eric Helgeson
  * ZuluSCSI™ - Copyright (c) 2022-2023 Rabbit Hole Computing™
+ * Portions - Copyright (C) 2023 Eric Helgeson
  *
  * This file is licensed under the GPL version 3 or any later version. 
  *

+ 36 - 39
src/QuirksCheck.cpp

@@ -34,33 +34,29 @@ static bool isValidMacintoshImage(image_config_t *img)
     const char apple_magic[2] = {0x45, 0x52};
     const char block_size[2]  = {0x02, 0x00};  // 512 BE == 2
     const char lido_sig[4] = {'C', 'M', 'S', '_' };
-    uint8_t tmp[4] = {0};
-
+    uint8_t tmp[SD_SECTOR_SIZE];
     // Check for Apple Magic
-    img->file.seek(0);
-    img->file.read(tmp, 2);
-    if(memcmp(apple_magic, tmp, 2) != 0)
+    img->file.seek(0); 
+    img->file.read(tmp, SD_SECTOR_SIZE);
+    if (memcmp(apple_magic, tmp, 2) != 0)
     {
         dbgmsg("---- Apple magic not found.");
         result = false;
     }
     // Check HFS Block size is 512
-    img->file.seek(2);
-    img->file.read(tmp, 2);
-    if(memcmp(block_size, tmp, 2) != 0)
+    if (memcmp(block_size, &tmp[2], 2) != 0)
     {
         dbgmsg("---- Block size not 512", block_size);
         result = false;
     }
-    // Find SCSI Driver offset
-    img->file.seek(MACINTOSH_SCSI_DRIVER_OFFSET);
-    img->file.read(tmp, 4);
-    uint64_t driver_offset_blocks = int((unsigned char)(tmp[0]) << 24 | (unsigned char)(tmp[1]) << 16 |
-                                        (unsigned char)(tmp[2]) << 8  |  (unsigned char)(tmp[3]));
+    uint8_t *mac_driver = &tmp[MACINTOSH_SCSI_DRIVER_OFFSET];
+    uint32_t driver_offset_blocks = mac_driver[0] << 24 | 
+                                    mac_driver[1] << 16 |
+                                    mac_driver[2] << 8  |  
+                                    mac_driver[3];
     // Find size of SCSI Driver partition
-    img->file.seek(MACINTOSH_SCSI_DRIVER_SIZE_OFFSET);
-    img->file.read(tmp, 2);
-    int driver_size_blocks = int((unsigned char)(tmp[0]) << 8 | (unsigned char)(tmp[1]));
+    uint8_t *mac_driver_size = &tmp[MACINTOSH_SCSI_DRIVER_SIZE_OFFSET];
+    uint32_t driver_size_blocks = mac_driver_size[0] << 8 | mac_driver_size[1];
     // SCSI Driver sanity checks
     if((driver_size_blocks * MACINTOSH_BLOCK_SIZE) > MACINTOSH_SCSI_DRIVER_MAX_SIZE ||
         (driver_offset_blocks * MACINTOSH_BLOCK_SIZE) > img->file.size())
@@ -69,10 +65,13 @@ static bool isValidMacintoshImage(image_config_t *img)
         result = false;
     }
     // Contains Lido Driver - driver causes issues on a Mac Plus and is generally slower than the Apple 4.3 or FWB.
-    // Also causes compatibility issues with other drivers.
-    img->file.seek(driver_offset_blocks * MACINTOSH_BLOCK_SIZE + LIDO_SIG_OFFSET);
-    img->file.read(tmp, 4);
-    if(memcmp(lido_sig, tmp, 4) == 0)
+    // Also causes compatibility issues with other drivers.   
+    // Mac block sizes should be the same size SD sector sizes for raw seeks, and reads to work 
+    assert(MACINTOSH_BLOCK_SIZE == SD_SECTOR_SIZE);
+    img->file.seek(driver_offset_blocks * MACINTOSH_BLOCK_SIZE); 
+    img->file.read(tmp, SD_SECTOR_SIZE);
+    uint8_t* lido_driver = &tmp[LIDO_SIG_OFFSET];
+    if(memcmp(lido_sig, lido_driver, 4) == 0)
     {
         logmsg("---- WARNING: This drive contains the LIDO driver and may cause issues.");
     }
@@ -83,38 +82,36 @@ static bool isValidMacintoshImage(image_config_t *img)
 // Called from ZuluSCSI_disk after image is initalized.
 static void macQuirksSanityCheck(image_config_t *img)
 {
+
     if(ini_getbool("SCSI", "DisableMacSanityCheck", false, CONFIGFILE))
     {
         dbgmsg("---- Skipping Mac sanity check due to DisableMacSanityCheck");
         return;
     }
-    if(img->file.isRom() || img->file.isRaw())
-    {
-        dbgmsg("---- Skipping Mac sanity check on ROM/Raw Drive.");
-        return;
-    }
-    if (img->quirks == S2S_CFG_QUIRKS_APPLE)
+
+    if(img->deviceType == S2S_CFG_FIXED)
     {
-        if(img->deviceType == S2S_CFG_FIXED)
+        if(!isValidMacintoshImage(img))
         {
-            if(!isValidMacintoshImage(img))
-            {
-                logmsg("---- WARNING: This image does not appear to be a valid Macintosh disk image.");
-            }
-            else
-            {
-                dbgmsg("---- Valid Macintosh disk image detected.");
-            }
+            logmsg("---- WARNING: This image does not appear to be a valid Macintosh disk image.");
         }
-        // Macintosh hosts reserve ID 7, so warn the user this configuration wont work
-        if((img->scsiId & S2S_CFG_TARGET_ID_BITS) == 7)
+        else
         {
-          logmsg("---- WARNING: Quirks set to Apple so can not use SCSI ID 7!");
+            dbgmsg("---- Valid Macintosh disk image detected.");
         }
     }
+    
+    // Macintosh hosts reserve ID 7, so warn the user this configuration wont work
+    if((img->scsiId & S2S_CFG_TARGET_ID_BITS) == 7)
+    {
+        logmsg("---- WARNING: Quirks set to Apple so can not use SCSI ID 7!");
+    }
 }
 
 void quirksCheck(image_config_t *img)
 {
-    macQuirksSanityCheck(img);
+    if (img->quirks == S2S_CFG_QUIRKS_APPLE)
+    {
+        macQuirksSanityCheck(img);
+    }
 }