From 71780449ff891a59eb0b0e3d53bf89032f29fe53 Mon Sep 17 00:00:00 2001 From: Uwe Seimet <48174652+uweseimet@users.noreply.github.com> Date: Tue, 14 Nov 2023 15:32:46 +0100 Subject: [PATCH] Move sector sizes lists from DeviceFactory to the respective devices (#1323) --- cpp/devices/device_factory.cpp | 52 ++++------------------- cpp/devices/device_factory.h | 29 +++++++++---- cpp/devices/disk.cpp | 6 +-- cpp/devices/disk.h | 14 +++---- cpp/devices/scsicd.cpp | 5 +-- cpp/devices/scsicd.h | 2 +- cpp/devices/scsihd.cpp | 6 +-- cpp/devices/scsihd.h | 2 +- cpp/devices/scsihd_nec.h | 2 +- cpp/devices/scsimo.cpp | 4 +- cpp/devices/scsimo.h | 2 +- cpp/piscsi/piscsi_core.cpp | 1 - cpp/piscsi/piscsi_core.h | 2 +- cpp/piscsi/piscsi_executor.cpp | 3 +- cpp/piscsi/piscsi_executor.h | 3 +- cpp/piscsi/piscsi_response.cpp | 69 ++++++++++++++++--------------- cpp/piscsi/piscsi_response.h | 7 ++-- cpp/test/device_factory_test.cpp | 35 ---------------- cpp/test/disk_test.cpp | 13 ++---- cpp/test/mocks.h | 12 ++++-- cpp/test/piscsi_executor_test.cpp | 10 +++-- cpp/test/scsicd_test.cpp | 24 +++++++---- cpp/test/scsihd_test.cpp | 27 ++++++++---- cpp/test/scsimo_test.cpp | 21 ++++++++-- 24 files changed, 160 insertions(+), 191 deletions(-) diff --git a/cpp/devices/device_factory.cpp b/cpp/devices/device_factory.cpp index f951d645..011a496b 100644 --- a/cpp/devices/device_factory.cpp +++ b/cpp/devices/device_factory.cpp @@ -7,7 +7,7 @@ // //--------------------------------------------------------------------------- -#include "shared/network_util.h" +#include "shared/piscsi_util.h" #include "scsihd.h" #include "scsihd_nec.h" #include "scsimo.h" @@ -20,39 +20,14 @@ using namespace std; using namespace piscsi_util; -using namespace network_util; - -DeviceFactory::DeviceFactory() -{ - sector_sizes[SCHD] = { 512, 1024, 2048, 4096 }; - sector_sizes[SCRM] = { 512, 1024, 2048, 4096 }; - sector_sizes[SCMO] = { 512, 1024, 2048, 4096 }; - sector_sizes[SCCD] = { 512, 2048}; - - extension_mapping["hd1"] = SCHD; - extension_mapping["hds"] = SCHD; - extension_mapping["hda"] = SCHD; - extension_mapping["hdn"] = SCHD; - extension_mapping["hdi"] = SCHD; - extension_mapping["nhd"] = SCHD; - extension_mapping["hdr"] = SCRM; - extension_mapping["mos"] = SCMO; - extension_mapping["iso"] = SCCD; - extension_mapping["is1"] = SCCD; - - device_mapping["bridge"] = SCBR; - device_mapping["daynaport"] = SCDP; - device_mapping["printer"] = SCLP; - device_mapping["services"] = SCHS; -} PbDeviceType DeviceFactory::GetTypeForFile(const string& filename) const { - if (const auto& it = extension_mapping.find(GetExtensionLowerCase(filename)); it != extension_mapping.end()) { + if (const auto& it = EXTENSION_MAPPING.find(GetExtensionLowerCase(filename)); it != EXTENSION_MAPPING.end()) { return it->second; } - if (const auto& it = device_mapping.find(filename); it != device_mapping.end()) { + if (const auto& it = DEVICE_MAPPING.find(filename); it != DEVICE_MAPPING.end()) { return it->second; } @@ -75,8 +50,7 @@ shared_ptr DeviceFactory::CreateDevice(PbDeviceType type, int lun if (const string ext = GetExtensionLowerCase(filename); ext == "hdn" || ext == "hdi" || ext == "nhd") { device = make_shared(lun); } else { - device = make_shared(lun, sector_sizes.find(type)->second, false, - ext == "hd1" ? scsi_level::scsi_1_ccs : scsi_level::scsi_2); + device = make_shared(lun, false, ext == "hd1" ? scsi_level::scsi_1_ccs : scsi_level::scsi_2); // Some Apple tools require a particular drive identification if (ext == "hda") { @@ -88,17 +62,17 @@ shared_ptr DeviceFactory::CreateDevice(PbDeviceType type, int lun } case SCRM: - device = make_shared(lun, sector_sizes.find(type)->second, true); + device = make_shared(lun, true, scsi_level::scsi_2); device->SetProduct("SCSI HD (REM.)"); break; case SCMO: - device = make_shared(lun, sector_sizes.find(type)->second); + device = make_shared(lun); device->SetProduct("SCSI MO"); break; case SCCD: - device = make_shared(lun, sector_sizes.find(type)->second, + device = make_shared(lun, GetExtensionLowerCase(filename) == "is1" ? scsi_level::scsi_1_ccs : scsi_level::scsi_2); device->SetProduct("SCSI CD-ROM"); break; @@ -135,15 +109,3 @@ shared_ptr DeviceFactory::CreateDevice(PbDeviceType type, int lun return device; } - -// TODO Move to respective device, which may require changes in the SCSI_HD/SCSIHD_NEC inheritance hierarchy -unordered_set DeviceFactory::GetSectorSizes(PbDeviceType type) const -{ - const auto& it = sector_sizes.find(type); - if (it != sector_sizes.end()) { - return it->second; - } - else { - return {}; - } -} diff --git a/cpp/devices/device_factory.h b/cpp/devices/device_factory.h index fef1c17d..7072980c 100644 --- a/cpp/devices/device_factory.h +++ b/cpp/devices/device_factory.h @@ -11,9 +11,7 @@ #pragma once -#include "shared/piscsi_util.h" #include -#include #include #include "generated/piscsi_interface.pb.h" @@ -27,19 +25,32 @@ class DeviceFactory public: - DeviceFactory(); + DeviceFactory() = default; ~DeviceFactory() = default; shared_ptr CreateDevice(PbDeviceType, int, const string&) const; PbDeviceType GetTypeForFile(const string&) const; - unordered_set GetSectorSizes(PbDeviceType type) const; - const auto& GetExtensionMapping() const { return extension_mapping; } + const auto& GetExtensionMapping() const { return EXTENSION_MAPPING; } private: - unordered_map> sector_sizes; + const inline static unordered_map> EXTENSION_MAPPING = { + { "hd1", SCHD }, + { "hds", SCHD }, + { "hda", SCHD }, + { "hdn", SCHD }, + { "hdi", SCHD }, + { "nhd", SCHD }, + { "hdr", SCRM }, + { "mos", SCMO }, + { "is1", SCCD }, + { "iso", SCCD } + }; - unordered_map> extension_mapping; - - unordered_map> device_mapping; + const inline static unordered_map> DEVICE_MAPPING = { + { "bridge", SCBR }, + { "daynaport", SCDP }, + { "printer", SCLP }, + { "services", SCHS } + }; }; diff --git a/cpp/devices/disk.cpp b/cpp/devices/disk.cpp index bc8b53d0..7ae8f275 100644 --- a/cpp/devices/disk.cpp +++ b/cpp/devices/disk.cpp @@ -695,7 +695,7 @@ uint32_t Disk::GetSectorSizeInBytes() const void Disk::SetSectorSizeInBytes(uint32_t size_in_bytes) { - if (DeviceFactory device_factory; !device_factory.GetSectorSizes(GetType()).contains(size_in_bytes)) { + if (!GetSupportedSectorSizes().contains(size_in_bytes)) { throw io_exception("Invalid sector size of " + to_string(size_in_bytes) + " byte(s)"); } @@ -708,9 +708,9 @@ uint32_t Disk::GetConfiguredSectorSize() const return configured_sector_size; } -bool Disk::SetConfiguredSectorSize(const DeviceFactory& device_factory, uint32_t configured_size) +bool Disk::SetConfiguredSectorSize(uint32_t configured_size) { - if (!device_factory.GetSectorSizes(GetType()).contains(configured_size)) { + if (!supported_sector_sizes.contains(configured_size)) { return false; } diff --git a/cpp/devices/disk.h b/cpp/devices/disk.h index 7364ad08..6bb54d66 100644 --- a/cpp/devices/disk.h +++ b/cpp/devices/disk.h @@ -16,7 +16,6 @@ #include "shared/scsi.h" #include "shared/piscsi_util.h" -#include "device_factory.h" #include "disk_track.h" #include "disk_cache.h" #include "interfaces/scsi_block_commands.h" @@ -35,8 +34,7 @@ class Disk : public StorageDevice, private ScsiBlockCommands unique_ptr cache; - // The supported configurable sector sizes, empty if not configurable - unordered_set sector_sizes; + unordered_set supported_sector_sizes; uint32_t configured_sector_size = 0; // Sector size shift count (9=512, 10=1024, 11=2048, 12=4096) @@ -50,7 +48,9 @@ class Disk : public StorageDevice, private ScsiBlockCommands public: - using StorageDevice::StorageDevice; + Disk(PbDeviceType type, int lun, const unordered_set& s) + : StorageDevice(type, lun), supported_sector_sizes(s) {} + ~Disk() override = default; bool Init(const param_map&) override; void CleanUp() override; @@ -64,8 +64,9 @@ public: virtual int Read(span , uint64_t); uint32_t GetSectorSizeInBytes() const; - bool IsSectorSizeConfigurable() const { return !sector_sizes.empty(); } - bool SetConfiguredSectorSize(const DeviceFactory&, uint32_t); + bool IsSectorSizeConfigurable() const { return supported_sector_sizes.size() > 1; } + const auto& GetSupportedSectorSizes() const { return supported_sector_sizes; } + bool SetConfiguredSectorSize(uint32_t); void FlushCache() override; vector GetStatistics() const override; @@ -119,7 +120,6 @@ protected: void AddCachePage(map>&, bool) const; unordered_set GetSectorSizes() const; - void SetSectorSizes(const unordered_set& sizes) { sector_sizes = sizes; } void SetSectorSizeInBytes(uint32_t); uint32_t GetSectorSizeShiftCount() const { return size_shift_count; } void SetSectorSizeShiftCount(uint32_t count) { size_shift_count = count; } diff --git a/cpp/devices/scsicd.cpp b/cpp/devices/scsicd.cpp index 200c36c3..27ab23a3 100644 --- a/cpp/devices/scsicd.cpp +++ b/cpp/devices/scsicd.cpp @@ -21,11 +21,8 @@ using namespace scsi_defs; using namespace scsi_command_util; -SCSICD::SCSICD(int lun, const unordered_set& sector_sizes, scsi_defs::scsi_level level) - : Disk(SCCD, lun), scsi_level(level) +SCSICD::SCSICD(int lun, scsi_defs::scsi_level level) : Disk(SCCD, lun, { 512, 2048 }), scsi_level(level) { - SetSectorSizes(sector_sizes); - SetReadOnly(true); SetRemovable(true); SetLockable(true); diff --git a/cpp/devices/scsicd.h b/cpp/devices/scsicd.h index db2620de..c2aaf4e3 100644 --- a/cpp/devices/scsicd.h +++ b/cpp/devices/scsicd.h @@ -25,7 +25,7 @@ class SCSICD : public Disk, private ScsiMmcCommands { public: - SCSICD(int, const unordered_set&, scsi_defs::scsi_level = scsi_level::scsi_2); + SCSICD(int, scsi_defs::scsi_level = scsi_level::scsi_2); ~SCSICD() override = default; bool Init(const param_map&) override; diff --git a/cpp/devices/scsihd.cpp b/cpp/devices/scsihd.cpp index 316c7575..09c6645a 100644 --- a/cpp/devices/scsihd.cpp +++ b/cpp/devices/scsihd.cpp @@ -19,11 +19,9 @@ using namespace scsi_command_util; -SCSIHD::SCSIHD(int lun, const unordered_set& sector_sizes, bool removable, scsi_defs::scsi_level level) - : Disk(removable ? SCRM : SCHD, lun), scsi_level(level) +SCSIHD::SCSIHD(int lun, bool removable, scsi_defs::scsi_level level, const unordered_set& sector_sizes) + : Disk(removable ? SCRM : SCHD, lun, sector_sizes), scsi_level(level) { - SetSectorSizes(sector_sizes); - SetProtectable(true); SetRemovable(removable); SetLockable(removable); diff --git a/cpp/devices/scsihd.h b/cpp/devices/scsihd.h index 8216ae2a..28a6119d 100644 --- a/cpp/devices/scsihd.h +++ b/cpp/devices/scsihd.h @@ -28,7 +28,7 @@ class SCSIHD : public Disk public: - SCSIHD(int, const unordered_set&, bool, scsi_defs::scsi_level = scsi_level::scsi_2); + SCSIHD(int, bool, scsi_defs::scsi_level, const unordered_set& = { 512, 1024, 2048, 4096 }); ~SCSIHD() override = default; void FinalizeSetup(off_t); diff --git a/cpp/devices/scsihd_nec.h b/cpp/devices/scsihd_nec.h index eec3c1be..4c2fabe2 100644 --- a/cpp/devices/scsihd_nec.h +++ b/cpp/devices/scsihd_nec.h @@ -33,7 +33,7 @@ class SCSIHD_NEC : public SCSIHD //NOSONAR The inheritance hierarchy depth is ac { public: - explicit SCSIHD_NEC(int lun) : SCSIHD(lun, { 512 }, false) {} + explicit SCSIHD_NEC(int lun) : SCSIHD(lun, false, scsi_level::scsi_1_ccs, { 512 }) {} ~SCSIHD_NEC() override = default; void Open() override; diff --git a/cpp/devices/scsimo.cpp b/cpp/devices/scsimo.cpp index 167064a1..57311be8 100644 --- a/cpp/devices/scsimo.cpp +++ b/cpp/devices/scsimo.cpp @@ -19,10 +19,8 @@ using namespace scsi_command_util; -SCSIMO::SCSIMO(int lun, const unordered_set& sector_sizes) : Disk(SCMO, lun) +SCSIMO::SCSIMO(int lun) : Disk(SCMO, lun, { 512, 1024, 2048, 4096 }) { - SetSectorSizes(sector_sizes); - // 128 MB, 512 bytes per sector, 248826 sectors geometries[512 * 248826] = { 512, 248826 }; // 230 MB, 512 bytes per block, 446325 sectors diff --git a/cpp/devices/scsimo.h b/cpp/devices/scsimo.h index 41735d3b..1ef651b8 100644 --- a/cpp/devices/scsimo.h +++ b/cpp/devices/scsimo.h @@ -26,7 +26,7 @@ class SCSIMO : public Disk { public: - SCSIMO(int, const unordered_set&); + explicit SCSIMO(int); ~SCSIMO() override = default; void Open() override; diff --git a/cpp/piscsi/piscsi_core.cpp b/cpp/piscsi/piscsi_core.cpp index 553e2297..bd491b8c 100644 --- a/cpp/piscsi/piscsi_core.cpp +++ b/cpp/piscsi/piscsi_core.cpp @@ -17,7 +17,6 @@ #include "shared/piscsi_version.h" #include "controllers/scsi_controller.h" #include "devices/device_logger.h" -#include "devices/device_factory.h" #include "devices/storage_device.h" #include "hal/gpiobus_factory.h" #include "hal/gpiobus.h" diff --git a/cpp/piscsi/piscsi_core.h b/cpp/piscsi/piscsi_core.h index 1560851d..8d8b917f 100644 --- a/cpp/piscsi/piscsi_core.h +++ b/cpp/piscsi/piscsi_core.h @@ -67,7 +67,7 @@ private: PiscsiImage piscsi_image; - PiscsiResponse response; + [[no_unique_address]] PiscsiResponse response; PiscsiService service; diff --git a/cpp/piscsi/piscsi_executor.cpp b/cpp/piscsi/piscsi_executor.cpp index d930184e..319623eb 100644 --- a/cpp/piscsi/piscsi_executor.cpp +++ b/cpp/piscsi/piscsi_executor.cpp @@ -10,7 +10,6 @@ #include "shared/piscsi_util.h" #include "shared/protobuf_util.h" #include "shared/piscsi_exceptions.h" -#include "devices/device_factory.h" #include "devices/disk.h" #include "localizer.h" #include "command_context.h" @@ -530,7 +529,7 @@ bool PiscsiExecutor::SetSectorSize(const CommandContext& context, shared_ptr(device); if (disk != nullptr && disk->IsSectorSizeConfigurable()) { - if (!disk->SetConfiguredSectorSize(device_factory, size)) { + if (!disk->SetConfiguredSectorSize(size)) { return context.ReturnLocalizedError(LocalizationKey::ERROR_BLOCK_SIZE, to_string(size)); } } diff --git a/cpp/piscsi/piscsi_executor.h b/cpp/piscsi/piscsi_executor.h index bae4fe60..6c4ea1a9 100644 --- a/cpp/piscsi/piscsi_executor.h +++ b/cpp/piscsi/piscsi_executor.h @@ -11,6 +11,7 @@ #include "hal/bus.h" #include "controllers/controller_manager.h" +#include "devices/device_factory.h" #include class DeviceFactory; @@ -60,7 +61,7 @@ private: ControllerManager& controller_manager; - const DeviceFactory device_factory; + [[no_unique_address]] const DeviceFactory device_factory; unordered_set reserved_ids; }; diff --git a/cpp/piscsi/piscsi_response.cpp b/cpp/piscsi/piscsi_response.cpp index 28e7e1a2..1f4d2fe0 100644 --- a/cpp/piscsi/piscsi_response.cpp +++ b/cpp/piscsi/piscsi_response.cpp @@ -24,26 +24,29 @@ using namespace piscsi_util; using namespace network_util; using namespace protobuf_util; -void PiscsiResponse::GetDeviceProperties(const Device& device, PbDeviceProperties& properties) const +void PiscsiResponse::GetDeviceProperties(shared_ptr device, PbDeviceProperties& properties) const { properties.set_luns(ControllerManager::GetScsiLunMax()); - properties.set_read_only(device.IsReadOnly()); - properties.set_protectable(device.IsProtectable()); - properties.set_stoppable(device.IsStoppable()); - properties.set_removable(device.IsRemovable()); - properties.set_lockable(device.IsLockable()); - properties.set_supports_file(device.SupportsFile()); - properties.set_supports_params(device.SupportsParams()); + properties.set_read_only(device->IsReadOnly()); + properties.set_protectable(device->IsProtectable()); + properties.set_stoppable(device->IsStoppable()); + properties.set_removable(device->IsRemovable()); + properties.set_lockable(device->IsLockable()); + properties.set_supports_file(device->SupportsFile()); + properties.set_supports_params(device->SupportsParams()); - if (device.SupportsParams()) { - for (const auto& [key, value] : device.GetDefaultParams()) { + if (device->SupportsParams()) { + for (const auto& [key, value] : device->GetDefaultParams()) { auto& map = *properties.mutable_default_params(); map[key] = value; } } - for (const auto& block_size : device_factory.GetSectorSizes(device.GetType())) { - properties.add_block_sizes(block_size); + shared_ptr disk = dynamic_pointer_cast(device); + if (disk != nullptr && disk->IsSectorSizeConfigurable()) { + for (const auto& sector_size : disk->GetSupportedSectorSizes()) { + properties.add_block_sizes(sector_size); + } } } @@ -52,7 +55,7 @@ void PiscsiResponse::GetDeviceTypeProperties(PbDeviceTypesInfo& device_types_inf auto type_properties = device_types_info.add_properties(); type_properties->set_type(type); const auto device = device_factory.CreateDevice(type, 0, ""); - GetDeviceProperties(*device, *type_properties->mutable_properties()); + GetDeviceProperties(device, *type_properties->mutable_properties()); } void PiscsiResponse::GetDeviceTypesInfo(PbDeviceTypesInfo& device_types_info) const @@ -67,37 +70,37 @@ void PiscsiResponse::GetDeviceTypesInfo(PbDeviceTypesInfo& device_types_info) co } } -void PiscsiResponse::GetDevice(const Device& device, PbDevice& pb_device, const string& default_folder) const +void PiscsiResponse::GetDevice(shared_ptr device, PbDevice& pb_device, const string& default_folder) const { - pb_device.set_id(device.GetId()); - pb_device.set_unit(device.GetLun()); - pb_device.set_vendor(device.GetVendor()); - pb_device.set_product(device.GetProduct()); - pb_device.set_revision(device.GetRevision()); - pb_device.set_type(device.GetType()); + pb_device.set_id(device->GetId()); + pb_device.set_unit(device->GetLun()); + pb_device.set_vendor(device->GetVendor()); + pb_device.set_product(device->GetProduct()); + pb_device.set_revision(device->GetRevision()); + pb_device.set_type(device->GetType()); GetDeviceProperties(device, *pb_device.mutable_properties()); auto status = pb_device.mutable_status(); - status->set_protected_(device.IsProtected()); - status->set_stopped(device.IsStopped()); - status->set_removed(device.IsRemoved()); - status->set_locked(device.IsLocked()); + status->set_protected_(device->IsProtected()); + status->set_stopped(device->IsStopped()); + status->set_removed(device->IsRemoved()); + status->set_locked(device->IsLocked()); - if (device.SupportsParams()) { - for (const auto& [key, value] : device.GetParams()) { + if (device->SupportsParams()) { + for (const auto& [key, value] : device->GetParams()) { SetParam(pb_device, key, value); } } - if (const auto disk = dynamic_cast(&device); disk) { - pb_device.set_block_size(device.IsRemoved()? 0 : disk->GetSectorSizeInBytes()); - pb_device.set_block_count(device.IsRemoved() ? 0: disk->GetBlockCount()); + if (const auto disk = dynamic_pointer_cast(device); disk) { + pb_device.set_block_size(device->IsRemoved()? 0 : disk->GetSectorSizeInBytes()); + pb_device.set_block_count(device->IsRemoved() ? 0: disk->GetBlockCount()); } - const auto storage_device = dynamic_cast(&device); + const auto storage_device = dynamic_pointer_cast(device); if (storage_device != nullptr) { - GetImageFile(*pb_device.mutable_file(), default_folder, device.IsReady() ? storage_device->GetFilename() : ""); + GetImageFile(*pb_device.mutable_file(), default_folder, device->IsReady() ? storage_device->GetFilename() : ""); } } @@ -191,7 +194,7 @@ void PiscsiResponse::GetDevices(const unordered_set>& { for (const auto& device : devices) { PbDevice *pb_device = server_info.mutable_devices_info()->add_devices(); - GetDevice(*device, *pb_device, default_folder); + GetDevice(device, *pb_device, default_folder); } } @@ -218,7 +221,7 @@ void PiscsiResponse::GetDevicesInfo(const unordered_setGetId() == id && d->GetLun() == lun) { - GetDevice(*d, *devices_info->add_devices(), default_folder); + GetDevice(d, *devices_info->add_devices(), default_folder); break; } } diff --git a/cpp/piscsi/piscsi_response.h b/cpp/piscsi/piscsi_response.h index b095416f..0015655c 100644 --- a/cpp/piscsi/piscsi_response.h +++ b/cpp/piscsi/piscsi_response.h @@ -47,11 +47,10 @@ private: inline static const vector EMPTY_VECTOR; - // TODO Try to get rid of this field by having the device instead of the factory providing the device data - const DeviceFactory device_factory; + [[no_unique_address]] const DeviceFactory device_factory; - void GetDeviceProperties(const Device&, PbDeviceProperties&) const; - void GetDevice(const Device&, PbDevice&, const string&) const; + void GetDeviceProperties(shared_ptr, PbDeviceProperties&) const; + void GetDevice(shared_ptr, PbDevice&, const string&) const; void GetDeviceTypeProperties(PbDeviceTypesInfo&, PbDeviceType) const; void GetAvailableImages(PbImageFilesInfo&, const string&, const string&, const string&, int) const; void GetAvailableImages(PbServerInfo&, const string&, const string&, const string&, int) const; diff --git a/cpp/test/device_factory_test.cpp b/cpp/test/device_factory_test.cpp index 0c51c194..ce74398b 100644 --- a/cpp/test/device_factory_test.cpp +++ b/cpp/test/device_factory_test.cpp @@ -39,41 +39,6 @@ TEST(DeviceFactoryTest, GetTypeForFile) EXPECT_EQ(device_factory.GetTypeForFile("test.iso.suffix"), UNDEFINED); } -TEST(DeviceFactoryTest, GetSectorSizes) -{ - DeviceFactory device_factory; - - unordered_set sector_sizes = device_factory.GetSectorSizes(SCHD); - EXPECT_EQ(4, sector_sizes.size()); - - EXPECT_TRUE(sector_sizes.contains(512)); - EXPECT_TRUE(sector_sizes.contains(1024)); - EXPECT_TRUE(sector_sizes.contains(2048)); - EXPECT_TRUE(sector_sizes.contains(4096)); - - sector_sizes = device_factory.GetSectorSizes(SCRM); - EXPECT_EQ(4, sector_sizes.size()); - - EXPECT_TRUE(sector_sizes.contains(512)); - EXPECT_TRUE(sector_sizes.contains(1024)); - EXPECT_TRUE(sector_sizes.contains(2048)); - EXPECT_TRUE(sector_sizes.contains(4096)); - - sector_sizes = device_factory.GetSectorSizes(SCMO); - EXPECT_EQ(4, sector_sizes.size()); - - EXPECT_TRUE(sector_sizes.contains(512)); - EXPECT_TRUE(sector_sizes.contains(1024)); - EXPECT_TRUE(sector_sizes.contains(2048)); - EXPECT_TRUE(sector_sizes.contains(4096)); - - sector_sizes = device_factory.GetSectorSizes(SCCD); - EXPECT_EQ(2, sector_sizes.size()); - - EXPECT_TRUE(sector_sizes.contains(512)); - EXPECT_TRUE(sector_sizes.contains(2048)); -} - TEST(DeviceFactoryTest, GetExtensionMapping) { DeviceFactory device_factory; diff --git a/cpp/test/disk_test.cpp b/cpp/test/disk_test.cpp index b0206403..f7ad298f 100644 --- a/cpp/test/disk_test.cpp +++ b/cpp/test/disk_test.cpp @@ -774,14 +774,8 @@ TEST(DiskTest, SectorSize) { MockDisk disk; - unordered_set sizes = { 512, 1024 }; - disk.SetSectorSizes(sizes); EXPECT_TRUE(disk.IsSectorSizeConfigurable()); - sizes.clear(); - disk.SetSectorSizes(sizes); - EXPECT_FALSE(disk.IsSectorSizeConfigurable()); - disk.SetSectorSizeShiftCount(9); EXPECT_EQ(9, disk.GetSectorSizeShiftCount()); EXPECT_EQ(512, disk.GetSectorSizeInBytes()); @@ -815,13 +809,12 @@ TEST(DiskTest, SectorSize) TEST(DiskTest, ConfiguredSectorSize) { - DeviceFactory device_factory; - MockSCSIHD disk(0, {}, false); + MockSCSIHD disk(0, false); - EXPECT_TRUE(disk.SetConfiguredSectorSize(device_factory, 512)); + EXPECT_TRUE(disk.SetConfiguredSectorSize(512)); EXPECT_EQ(512, disk.GetConfiguredSectorSize()); - EXPECT_FALSE(disk.SetConfiguredSectorSize(device_factory, 1234)); + EXPECT_FALSE(disk.SetConfiguredSectorSize(1234)); EXPECT_EQ(512, disk.GetConfiguredSectorSize()); } diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index fcf45cd5..cc761802 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -352,7 +352,7 @@ public: MOCK_METHOD(void, FlushCache, (), (override)); MOCK_METHOD(void, Open, (), (override)); - MockDisk() : Disk(SCHD, 0) {} + MockDisk() : Disk(SCHD, 0, { 512, 1024, 2048, 4096 }) {} ~MockDisk() override = default; }; @@ -363,10 +363,15 @@ class MockSCSIHD : public SCSIHD //NOSONAR Ignore inheritance hierarchy depth in FRIEND_TEST(ScsiHdTest, FinalizeSetup); FRIEND_TEST(ScsiHdTest, GetProductData); FRIEND_TEST(ScsiHdTest, SetUpModePages); - FRIEND_TEST(PiscsiExecutorTest, SetSectorSize); + FRIEND_TEST(ScsiHdTest, GetSectorSizes); FRIEND_TEST(ScsiHdTest, ModeSelect); + FRIEND_TEST(PiscsiExecutorTest, SetSectorSize); - using SCSIHD::SCSIHD; +public: + + MockSCSIHD(int lun, bool removable) : SCSIHD(lun, removable, scsi_level::scsi_2) {} + explicit MockSCSIHD(const unordered_set& sector_sizes) : SCSIHD(0, false, scsi_level::scsi_2, sector_sizes) {} + ~MockSCSIHD() override = default; }; class MockSCSIHD_NEC : public SCSIHD_NEC //NOSONAR Ignore inheritance hierarchy depth in unit tests @@ -382,6 +387,7 @@ class MockSCSIHD_NEC : public SCSIHD_NEC //NOSONAR Ignore inheritance hierarchy class MockSCSICD : public SCSICD //NOSONAR Ignore inheritance hierarchy depth in unit tests { + FRIEND_TEST(ScsiCdTest, GetSectorSizes); FRIEND_TEST(ScsiCdTest, SetUpModePages); FRIEND_TEST(ScsiCdTest, ReadToc); diff --git a/cpp/test/piscsi_executor_test.cpp b/cpp/test/piscsi_executor_test.cpp index d6557be4..06b218a1 100644 --- a/cpp/test/piscsi_executor_test.cpp +++ b/cpp/test/piscsi_executor_test.cpp @@ -252,8 +252,6 @@ TEST(PiscsiExecutorTest, Attach) TEST(PiscsiExecutorTest, Insert) { - DeviceFactory device_factory; - auto bus = make_shared(); ControllerManager controller_manager; auto [controller, device] = CreateDevice(SCHD); @@ -500,13 +498,17 @@ TEST(PiscsiExecutorTest, SetSectorSize) CommandContext context(command, "", ""); unordered_set sizes; - auto hd = make_shared(0, sizes, false); + auto hd = make_shared(sizes); EXPECT_FALSE(executor.SetSectorSize(context, hd, 512)); sizes.insert(512); - hd = make_shared(0, sizes, false); + hd = make_shared(sizes); EXPECT_TRUE(executor.SetSectorSize(context, hd, 0)); EXPECT_FALSE(executor.SetSectorSize(context, hd, 1)); + EXPECT_FALSE(executor.SetSectorSize(context, hd, 512)); + + sizes.insert(1024); + hd = make_shared(sizes); EXPECT_TRUE(executor.SetSectorSize(context, hd, 512)); } diff --git a/cpp/test/scsicd_test.cpp b/cpp/test/scsicd_test.cpp index fa5d8743..b4c9154f 100644 --- a/cpp/test/scsicd_test.cpp +++ b/cpp/test/scsicd_test.cpp @@ -34,10 +34,21 @@ TEST(ScsiCdTest, Inquiry) TestInquiry::Inquiry(SCCD, device_type::cd_rom, scsi_level::scsi_1_ccs, "PiSCSI SCSI CD-ROM ", 0x1f, true, "file.is1"); } +TEST(ScsiCdTest, GetSectorSizes) +{ + MockSCSICD cd(0); + + const auto& sector_sizes = cd.GetSupportedSectorSizes(); + EXPECT_EQ(2, sector_sizes.size()); + + EXPECT_TRUE(sector_sizes.contains(512)); + EXPECT_TRUE(sector_sizes.contains(2048)); +} + TEST(ScsiCdTest, SetUpModePages) { map> pages; - MockSCSICD cd(0, {}); + MockSCSICD cd(0); // Non changeable cd.SetUpModePages(pages, 0x3f, false); @@ -51,10 +62,10 @@ TEST(ScsiCdTest, SetUpModePages) TEST(ScsiCdTest, Open) { - MockSCSICD cd_iso(0, {}); - MockSCSICD cd_cue(0, {}); - MockSCSICD cd_raw(0, {}); - MockSCSICD cd_physical(0, {}); + MockSCSICD cd_iso(0); + MockSCSICD cd_cue(0); + MockSCSICD cd_raw(0); + MockSCSICD cd_physical(0); EXPECT_THROW(cd_iso.Open(), io_exception) << "Missing filename"; @@ -111,8 +122,7 @@ TEST(ScsiCdTest, Open) TEST(ScsiCdTest, ReadToc) { auto controller = make_shared(); - const unordered_set sector_sizes; - auto cd = make_shared(0, sector_sizes); + auto cd = make_shared(0); EXPECT_TRUE(cd->Init({})); controller->AddDevice(cd); diff --git a/cpp/test/scsihd_test.cpp b/cpp/test/scsihd_test.cpp index 1cdacad0..693f450b 100644 --- a/cpp/test/scsihd_test.cpp +++ b/cpp/test/scsihd_test.cpp @@ -30,14 +30,14 @@ TEST(ScsiHdTest, Inquiry) TEST(ScsiHdTest, SupportsSaveParameters) { - MockSCSIHD hd(0, {}, false); + MockSCSIHD hd(0, false); EXPECT_TRUE(hd.SupportsSaveParameters()); } TEST(ScsiHdTest, FinalizeSetup) { - MockSCSIHD hd(0, {}, false); + MockSCSIHD hd(0, false); hd.SetSectorSizeInBytes(1024); EXPECT_THROW(hd.FinalizeSetup(0), io_exception) << "Device has 0 blocks"; @@ -45,9 +45,9 @@ TEST(ScsiHdTest, FinalizeSetup) TEST(ScsiHdTest, GetProductData) { - MockSCSIHD hd_kb(0, {}, false); - MockSCSIHD hd_mb(0, {}, false); - MockSCSIHD hd_gb(0, {}, false); + MockSCSIHD hd_kb(0, false); + MockSCSIHD hd_mb(0, false); + MockSCSIHD hd_gb(0, false); const path filename = CreateTempFile(1); hd_kb.SetFilename(string(filename)); @@ -73,10 +73,23 @@ TEST(ScsiHdTest, GetProductData) remove(filename); } +TEST(ScsiHdTest, GetSectorSizes) +{ + MockSCSIHD hd(0, false); + + const auto& sector_sizes = hd.GetSupportedSectorSizes(); + EXPECT_EQ(4, sector_sizes.size()); + + EXPECT_TRUE(sector_sizes.contains(512)); + EXPECT_TRUE(sector_sizes.contains(1024)); + EXPECT_TRUE(sector_sizes.contains(2048)); + EXPECT_TRUE(sector_sizes.contains(4096)); +} + TEST(ScsiHdTest, SetUpModePages) { map> pages; - MockSCSIHD hd(0, {}, false); + MockSCSIHD hd(0, false); // Non changeable hd.SetUpModePages(pages, 0x3f, false); @@ -90,7 +103,7 @@ TEST(ScsiHdTest, SetUpModePages) TEST(ScsiHdTest, ModeSelect) { - MockSCSIHD hd(0, { 512 }, false); + MockSCSIHD hd({ 512 }); vector cmd(10); vector buf(255); diff --git a/cpp/test/scsimo_test.cpp b/cpp/test/scsimo_test.cpp index 2422c479..d38ee4a4 100644 --- a/cpp/test/scsimo_test.cpp +++ b/cpp/test/scsimo_test.cpp @@ -28,15 +28,28 @@ TEST(ScsiMoTest, Inquiry) TEST(ScsiMoTest, SupportsSaveParameters) { map> pages; - MockSCSIMO mo(0, {}); + MockSCSIMO mo(0); EXPECT_TRUE(mo.SupportsSaveParameters()); } +TEST(ScsiMoTest, GetSectorSizes) +{ + MockSCSIMO mo(0); + + const auto& sector_sizes = mo.GetSupportedSectorSizes(); + EXPECT_EQ(4, sector_sizes.size()); + + EXPECT_TRUE(sector_sizes.contains(512)); + EXPECT_TRUE(sector_sizes.contains(1024)); + EXPECT_TRUE(sector_sizes.contains(2048)); + EXPECT_TRUE(sector_sizes.contains(4096)); +} + TEST(ScsiMoTest, SetUpModePages) { map> pages; - MockSCSIMO mo(0, {}); + MockSCSIMO mo(0); // Non changeable mo.SetUpModePages(pages, 0x3f, false); @@ -51,7 +64,7 @@ TEST(ScsiMoTest, SetUpModePages) TEST(ScsiMoTest, TestAddVendorPage) { map> pages; - MockSCSIMO mo(0, {}); + MockSCSIMO mo(0); mo.SetReady(true); mo.SetUpModePages(pages, 0x21, false); @@ -122,7 +135,7 @@ TEST(ScsiMoTest, TestAddVendorPage) TEST(ScsiMoTest, ModeSelect) { - MockSCSIMO mo(0, { 1024, 2048 }); + MockSCSIMO mo(0); vector cmd(10); vector buf(255);