From a6a8cadf21b8b4f103190798deed67ba9a9e429f Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Sat, 13 Apr 2024 19:40:53 +0900 Subject: [PATCH] Revert fixes for DEC vendor specific pages and CD-ROM block size changing (#1451) * Revert "Don't ResizeCache on sector change if no filename is defined (#1438)" This reverts commit dd9a3296d4f0060b923bf1297c5849cfae297333. * Revert "Add ModeSense page 0x25 (DEC special function control page) (#1412)" This reverts commit 1121b8d9d699468f792ea1c1c484a25d31a367b1. * Revert "DiskCache needs a size" This reverts commit 7cc8df271cfcfb5627d7ed7f7aa853c23acbfe01. * Revert "Honor sector size change via ModeSelect6 in scsicd (#1406)" This reverts commit b7f65d33e237c4aaf9272c2af7447612a8494ff0. * Revert "Multiple fixes for ModeSelect (#1405)" This reverts commit ad5eae93e7f721817c95a01285babdb97ed940b8. --- cpp/devices/disk.cpp | 10 ++---- cpp/devices/disk.h | 2 +- cpp/devices/disk_cache.h | 1 - cpp/devices/mode_page_device.cpp | 2 +- cpp/devices/mode_page_device.h | 2 +- cpp/devices/scsi_command_util.cpp | 9 ++--- cpp/devices/scsicd.cpp | 27 --------------- cpp/devices/scsicd.h | 1 - cpp/devices/scsihd.cpp | 26 +------------- cpp/devices/scsihd.h | 3 +- cpp/devices/scsimo.cpp | 2 +- cpp/devices/scsimo.h | 2 +- cpp/test/mocks.h | 2 -- cpp/test/scsi_command_util_test.cpp | 27 ++------------- cpp/test/scsicd_test.cpp | 53 ----------------------------- cpp/test/scsihd_nec_test.cpp | 3 +- cpp/test/scsihd_test.cpp | 17 +-------- 17 files changed, 16 insertions(+), 173 deletions(-) diff --git a/cpp/devices/disk.cpp b/cpp/devices/disk.cpp index b771b881..7ae8f275 100644 --- a/cpp/devices/disk.cpp +++ b/cpp/devices/disk.cpp @@ -696,17 +696,11 @@ uint32_t Disk::GetSectorSizeInBytes() const void Disk::SetSectorSizeInBytes(uint32_t size_in_bytes) { if (!GetSupportedSectorSizes().contains(size_in_bytes)) { - throw io_exception("Invalid sector size of " + to_string(size_in_bytes) + " byte(s)"); + throw io_exception("Invalid sector size of " + to_string(size_in_bytes) + " byte(s)"); } - uint64_t current_blocks = GetBlockCount(); - uint32_t current_size_shift_count = size_shift_count; - uint64_t current_size = current_blocks << current_size_shift_count; size_shift_count = CalculateShiftCount(size_in_bytes); assert(size_shift_count); - if ((current_blocks > 0) && (current_size_shift_count > 0)) { - SetBlockCount(current_size >> size_shift_count); - } } uint32_t Disk::GetConfiguredSectorSize() const @@ -720,7 +714,7 @@ bool Disk::SetConfiguredSectorSize(uint32_t configured_size) return false; } - configured_sector_size = configured_size; + configured_sector_size = configured_size; return true; } diff --git a/cpp/devices/disk.h b/cpp/devices/disk.h index a69a4662..6bb54d66 100644 --- a/cpp/devices/disk.h +++ b/cpp/devices/disk.h @@ -112,7 +112,7 @@ protected: void SetUpCache(off_t, bool = false); void ResizeCache(const string&, bool); - bool GetRawMode() const { return (cache?cache->GetRawMode():false); } + void SetUpModePages(map>&, int, bool) const override; void AddErrorPage(map>&, bool) const; virtual void AddFormatPage(map>&, bool) const; diff --git a/cpp/devices/disk_cache.h b/cpp/devices/disk_cache.h index d25eb2f0..ec486edd 100644 --- a/cpp/devices/disk_cache.h +++ b/cpp/devices/disk_cache.h @@ -51,7 +51,6 @@ public: ~DiskCache() = default; void SetRawMode(bool b) { cd_raw = b; } // CD-ROM raw mode setting - bool GetRawMode() const { return cd_raw; } bool Save(); // Save and release all bool ReadSector(span, uint32_t); // Sector Read diff --git a/cpp/devices/mode_page_device.cpp b/cpp/devices/mode_page_device.cpp index bb64cc37..7335d5a3 100644 --- a/cpp/devices/mode_page_device.cpp +++ b/cpp/devices/mode_page_device.cpp @@ -114,7 +114,7 @@ void ModePageDevice::ModeSense10() const EnterDataInPhase(); } -void ModePageDevice::ModeSelect(scsi_command, cdb_t, span, int) +void ModePageDevice::ModeSelect(scsi_command, cdb_t, span, int) const { // There is no default implementation of MODE SELECT throw scsi_exception(sense_key::illegal_request, asc::invalid_command_operation_code); diff --git a/cpp/devices/mode_page_device.h b/cpp/devices/mode_page_device.h index 4b657ebe..4a487fb2 100644 --- a/cpp/devices/mode_page_device.h +++ b/cpp/devices/mode_page_device.h @@ -23,7 +23,7 @@ public: bool Init(const param_map&) override; - virtual void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int); + virtual void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) const; protected: diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp index 5c6068a7..20bb83f7 100644 --- a/cpp/devices/scsi_command_util.cpp +++ b/cpp/devices/scsi_command_util.cpp @@ -40,8 +40,7 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span 0) { @@ -63,10 +62,6 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span SCSICD::InquiryInternal() const return HandleInquiry(device_type::cd_rom, scsi_level, true); } -void SCSICD::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) -{ - int sector_size = 1 << GetSectorSizeShiftCount(); - int wanted_sector_size; - // skip Block Descriptor - int offset = 4; - // evaluate Mode Parameter Block Descriptor, sector size - wanted_sector_size = scsi_command_util::GetInt16(buf, offset + 6); - if (wanted_sector_size != sector_size) { - LogDebug("Changing sector size from " + to_string(sector_size) + " to " + to_string(wanted_sector_size)); - SetSectorSizeInBytes(wanted_sector_size); - ClearTrack(); - CreateDataTrack(); - FlushCache(); - string filename; - if ((filename = GetFilename()) != "") { - // DiskCache fails without a file to compute the cache size - ResizeCache(filename, GetRawMode()); - } - } - - if (const string result = scsi_command_util::ModeSelect(cmd, cdb, buf, length, sector_size); - !result.empty()) { - LogWarn(result); - } -} - void SCSICD::SetUpModePages(map>& pages, int page, bool changeable) const { Disk::SetUpModePages(pages, page, changeable); diff --git a/cpp/devices/scsicd.h b/cpp/devices/scsicd.h index cb1158dd..c2aaf4e3 100644 --- a/cpp/devices/scsicd.h +++ b/cpp/devices/scsicd.h @@ -34,7 +34,6 @@ public: vector InquiryInternal() const override; int Read(span, uint64_t) override; - void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) override; protected: diff --git a/cpp/devices/scsihd.cpp b/cpp/devices/scsihd.cpp index ed3bc153..09c6645a 100644 --- a/cpp/devices/scsihd.cpp +++ b/cpp/devices/scsihd.cpp @@ -82,7 +82,7 @@ vector SCSIHD::InquiryInternal() const return HandleInquiry(device_type::direct_access, scsi_level, IsRemovable()); } -void SCSIHD::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) +void SCSIHD::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) const { if (const string result = scsi_command_util::ModeSelect(cmd, cdb, buf, length, 1 << GetSectorSizeShiftCount()); !result.empty()) { @@ -97,32 +97,8 @@ void SCSIHD::AddFormatPage(map>& pages, bool changeable) const EnrichFormatPage(pages, changeable, 1 << GetSectorSizeShiftCount()); } -// Page code 37 (25h) - DEC Special Function Control page - -void SCSIHD::AddDECSpecialFunctionControlPage(map>& pages, bool changeable) const -{ - vector buf(25); - - // No changeable area - if (changeable) { - pages[0x25] = buf; - - return; - } - - buf[0] = static_cast (0x25 | 0x80); // page code, high bit set - buf[1] = static_cast (sizeof(buf) - 1); - buf[2] = static_cast (0x01); // drive does not auto-start - - pages[0x25] = buf; -} - void SCSIHD::AddVendorPage(map>& pages, int page, bool changeable) const { - // Page code 0x25: DEC Special Function Control page - if (page == 0x25 || page == 0x3f) { - AddDECSpecialFunctionControlPage(pages, changeable); - } // Page code 48 if (page == 0x30 || page == 0x3f) { AddAppleVendorModePage(pages, changeable); diff --git a/cpp/devices/scsihd.h b/cpp/devices/scsihd.h index cd00191a..28a6119d 100644 --- a/cpp/devices/scsihd.h +++ b/cpp/devices/scsihd.h @@ -37,10 +37,9 @@ public: // Commands vector InquiryInternal() const override; - void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) override; + void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) const override; void AddFormatPage(map>&, bool) const override; - void AddDECSpecialFunctionControlPage(map>&, bool) const; void AddVendorPage(map>&, int, bool) const override; private: diff --git a/cpp/devices/scsimo.cpp b/cpp/devices/scsimo.cpp index ab2af6c4..57311be8 100644 --- a/cpp/devices/scsimo.cpp +++ b/cpp/devices/scsimo.cpp @@ -88,7 +88,7 @@ void SCSIMO::AddOptionPage(map>& pages, bool) const // Do not report update blocks } -void SCSIMO::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) +void SCSIMO::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) const { if (const string result = scsi_command_util::ModeSelect(cmd, cdb, buf, length, 1 << GetSectorSizeShiftCount()); !result.empty()) { diff --git a/cpp/devices/scsimo.h b/cpp/devices/scsimo.h index b41c193c..1ef651b8 100644 --- a/cpp/devices/scsimo.h +++ b/cpp/devices/scsimo.h @@ -32,7 +32,7 @@ public: void Open() override; vector InquiryInternal() const override; - void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) override; + void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) const override; protected: diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index dd3c8268..3fefdb40 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -360,7 +360,6 @@ class MockSCSIHD : public SCSIHD //NOSONAR Ignore inheritance hierarchy depth in FRIEND_TEST(ScsiHdTest, FinalizeSetup); FRIEND_TEST(ScsiHdTest, GetProductData); FRIEND_TEST(ScsiHdTest, SetUpModePages); - FRIEND_TEST(ScsiHdTest, DECSpecialFunctionControlPage); FRIEND_TEST(ScsiHdTest, GetSectorSizes); FRIEND_TEST(ScsiHdTest, ModeSelect); FRIEND_TEST(PiscsiExecutorTest, SetSectorSize); @@ -388,7 +387,6 @@ class MockSCSICD : public SCSICD //NOSONAR Ignore inheritance hierarchy depth in FRIEND_TEST(ScsiCdTest, GetSectorSizes); FRIEND_TEST(ScsiCdTest, SetUpModePages); FRIEND_TEST(ScsiCdTest, ReadToc); - FRIEND_TEST(ScsiCdTest, ModeSelect); using SCSICD::SCSICD; }; diff --git a/cpp/test/scsi_command_util_test.cpp b/cpp/test/scsi_command_util_test.cpp index 7bf81550..20afa389 100644 --- a/cpp/test/scsi_command_util_test.cpp +++ b/cpp/test/scsi_command_util_test.cpp @@ -46,11 +46,6 @@ TEST(ScsiCommandUtilTest, ModeSelect6) Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list)))) << "Unsupported page 0 was not rejected"; - // Page 1 - buf[12] = 0x01; - EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512)) - << "Page 1 is supported"; - // Page 3 (Format Device Page) buf[12] = 0x03; EXPECT_THAT([&] { ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512); }, @@ -67,25 +62,7 @@ TEST(ScsiCommandUtilTest, ModeSelect6) Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list)))) << "Not enough command parameters"; - // check length computation - buf[3] = 8; - buf[10] = 2; - buf[12] = 1; - buf[13] = 10; - buf[14] = 0x24; - buf[24] = 0; - EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512)) - << "Multi-page length computation"; - - // check length computation - buf[3] = 8; - buf[10] = 12; - buf[12] = 0; - buf[13] = 0; - buf[14] = 0; - buf[24] = 0; - EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, 12, 512)) - << "Empty ModeSelect6"; + EXPECT_FALSE(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512).empty()); } TEST(ScsiCommandUtilTest, ModeSelect10) @@ -134,6 +111,8 @@ TEST(ScsiCommandUtilTest, ModeSelect10) Property(&scsi_exception::get_sense_key, sense_key::illegal_request), Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list)))) << "Not enough command parameters"; + + EXPECT_FALSE(ModeSelect(scsi_command::eCmdModeSelect10, cdb, buf, LENGTH, 512).empty()); } TEST(ScsiCommandUtilTest, EnrichFormatPage) diff --git a/cpp/test/scsicd_test.cpp b/cpp/test/scsicd_test.cpp index cbef92bd..b4c9154f 100644 --- a/cpp/test/scsicd_test.cpp +++ b/cpp/test/scsicd_test.cpp @@ -133,56 +133,3 @@ TEST(ScsiCdTest, ReadToc) // Further testing requires filesystem access } - -TEST(ScsiCdTest, ModeSelect) -{ - MockSCSICD cd(0); - MockSCSICD cd1(0); - vector cmd(6); - vector buf(255); - - // dummy file for DiskCache resize after sector size change - path filename = CreateTempFile(2* 2048); - cd.SetFilename(string(filename)); - cd.Open(); - EXPECT_EQ(2, cd.GetBlockCount()); - - cd.SetSectorSizeInBytes(2048); - - // PF - cmd[1] = 0x10; - // Length - buf[3] = 0x08; - // 2048 bytes per sector - buf[10] = 0x08; - // Page 3 (Device Format Page) - buf[12] = 0x01; - EXPECT_NO_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255)) << "MODE SELECT(6) with sector size 2048 is supported"; - - // 512 bytes per sector - ModeSelect6 - buf[10] = 0x02; - EXPECT_NO_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255)) << "MODE SELECT(6) with sector size 512 is supported"; - - // 2048 bytes per sector - ModeSelect6 - buf[10] = 0x08; - EXPECT_NO_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255)) << "MODE SELECT(6) with sector size 2048 is supported"; - - // 512 bytes per sector - ModeSelect10 - buf[10] = 0x02; - EXPECT_NO_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect10, cmd, buf, 255)) << "MODE SELECT(10) with sector size 512 is supported"; - - // unsupported sector size - ModeSelect6 - buf[10] = 0x04; - EXPECT_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255), io_exception) << "MODE SELECT(6) with sector size 1024 is unsupported"; - - // sector size not multiple of 512 - ModeSelect6 - buf[10] = 0x03; - EXPECT_THROW(cd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255), io_exception) << "MODE SELECT(6) with sector size 768 is unsupported"; - - // cd1 has no dummy file attached, simulating an empty CD drive - cd1.SetSectorSizeInBytes(2048); - - // 512 bytes per sector - ModeSelect6 - buf[10] = 0x02; - EXPECT_NO_THROW(cd1.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, 255)) << "MODE SELECT(6), emtpy drive, with sector size 512 is supported"; -} diff --git a/cpp/test/scsihd_nec_test.cpp b/cpp/test/scsihd_nec_test.cpp index bbcfed39..6b5cbd7c 100644 --- a/cpp/test/scsihd_nec_test.cpp +++ b/cpp/test/scsihd_nec_test.cpp @@ -18,12 +18,11 @@ using namespace filesystem; void ScsiHdNecTest_SetUpModePages(map>& pages) { - EXPECT_EQ(6, pages.size()) << "Unexpected number of mode pages"; + EXPECT_EQ(5, pages.size()) << "Unexpected number of mode pages"; EXPECT_EQ(12, pages[1].size()); EXPECT_EQ(24, pages[3].size()); EXPECT_EQ(20, pages[4].size()); EXPECT_EQ(12, pages[8].size()); - EXPECT_EQ(25, pages[37].size()); EXPECT_EQ(30, pages[48].size()); } diff --git a/cpp/test/scsihd_test.cpp b/cpp/test/scsihd_test.cpp index 81efae30..693f450b 100644 --- a/cpp/test/scsihd_test.cpp +++ b/cpp/test/scsihd_test.cpp @@ -13,12 +13,11 @@ void ScsiHdTest_SetUpModePages(map>& pages) { - EXPECT_EQ(6, pages.size()) << "Unexpected number of mode pages"; + EXPECT_EQ(5, pages.size()) << "Unexpected number of mode pages"; EXPECT_EQ(12, pages[1].size()); EXPECT_EQ(24, pages[3].size()); EXPECT_EQ(24, pages[4].size()); EXPECT_EQ(12, pages[8].size()); - EXPECT_EQ(25, pages[37].size()); EXPECT_EQ(30, pages[48].size()); } @@ -102,20 +101,6 @@ TEST(ScsiHdTest, SetUpModePages) ScsiHdTest_SetUpModePages(pages); } -TEST(ScsiHdTest, DECSpecialFunctionControlPage) -{ - map> pages; - vector buf; - MockSCSIHD hd(0, false); - - EXPECT_NO_THROW(hd.SetUpModePages(pages, 0x25, false)) << "MODE SENSE(6) DEC unique page is supported"; - EXPECT_NE(pages.end(), pages.find(0x25)); - buf = pages[0x25]; - EXPECT_EQ(static_cast (0x25 | 0x80), buf[0]); - EXPECT_EQ(static_cast (0x17), buf[1]); - EXPECT_EQ(static_cast (0x01), buf[2]); -} - TEST(ScsiHdTest, ModeSelect) { MockSCSIHD hd({ 512 });