Move sector sizes lists from DeviceFactory to the respective devices (#…

…1323)
PiSCSI · Nov 14, 2023 · 6525766 · 6525766
1 parent 3a40b27
commit 6525766
Show file tree

Hide file tree

Showing 24 changed files with 162 additions and 193 deletions.
diff --git 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<PrimaryDevice> DeviceFactory::CreateDevice(PbDeviceType type, int lun
 		if (const string ext = GetExtensionLowerCase(filename); ext == "hdn" || ext == "hdi" || ext == "nhd") {
 			device = make_shared<SCSIHD_NEC>(lun);
 		} else {
-			device = make_shared<SCSIHD>(lun, sector_sizes.find(type)->second, false,
-					ext == "hd1" ? scsi_level::scsi_1_ccs : scsi_level::scsi_2);
+			device = make_shared<SCSIHD>(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<PrimaryDevice> DeviceFactory::CreateDevice(PbDeviceType type, int lun
 	}
 
 	case SCRM:
-		device = make_shared<SCSIHD>(lun, sector_sizes.find(type)->second, true);
+		device = make_shared<SCSIHD>(lun, true, scsi_level::scsi_2);
 		device->SetProduct("SCSI HD (REM.)");
 		break;
 
 	case SCMO:
-		device = make_shared<SCSIMO>(lun, sector_sizes.find(type)->second);
+		device = make_shared<SCSIMO>(lun);
 		device->SetProduct("SCSI MO");
 		break;
 
 	case SCCD:
-		device = make_shared<SCSICD>(lun, sector_sizes.find(type)->second,
+		device = make_shared<SCSICD>(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<PrimaryDevice> 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<uint32_t> 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
@@ -11,9 +11,7 @@
 
 #pragma once
 
-#include "shared/piscsi_util.h"
 #include <string>
-#include <unordered_set>
 #include <unordered_map>
 #include "generated/piscsi_interface.pb.h"
 
@@ -27,19 +25,32 @@ class DeviceFactory
 
 public:
 
-	DeviceFactory();
+	DeviceFactory() = default;
 	~DeviceFactory() = default;
 
 	shared_ptr<PrimaryDevice> CreateDevice(PbDeviceType, int, const string&) const;
 	PbDeviceType GetTypeForFile(const string&) const;
-	unordered_set<uint32_t> GetSectorSizes(PbDeviceType type) const;
-	const auto& GetExtensionMapping() const { return extension_mapping; }
+	const auto& GetExtensionMapping() const { return EXTENSION_MAPPING; }
 
 private:
 
-	unordered_map<PbDeviceType, unordered_set<uint32_t>> sector_sizes;
-
-	unordered_map<string, PbDeviceType, piscsi_util::StringHash, equal_to<>> extension_mapping;
-
-	unordered_map<string, PbDeviceType, piscsi_util::StringHash, equal_to<>> device_mapping;
+	const inline static unordered_map<string, PbDeviceType, piscsi_util::StringHash, equal_to<>> EXTENSION_MAPPING = {
+			{ "hd1", SCHD },
+			{ "hds", SCHD },
+			{ "hda", SCHD },
+			{ "hdn", SCHD },
+			{ "hdi", SCHD },
+			{ "nhd", SCHD },
+			{ "hdr", SCRM },
+			{ "mos", SCMO },
+			{ "is1", SCCD },
+			{ "iso", SCCD }
+	};
+
+	const inline static unordered_map<string, PbDeviceType, piscsi_util::StringHash, equal_to<>> DEVICE_MAPPING = {
+			{ "bridge", SCBR },
+			{ "daynaport", SCDP },
+			{ "printer", SCLP },
+			{ "services", SCHS }
+	};
 };
diff --git 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
@@ -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<DiskCache> cache;
 
-	// The supported configurable sector sizes, empty if not configurable
-	unordered_set<uint32_t> sector_sizes;
+	unordered_set<uint32_t> 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<uint32_t>& s)
+		: StorageDevice(type, lun), supported_sector_sizes(s) {}
+	~Disk() override = default;
 
 	bool Init(const param_map&) override;
 	void CleanUp() override;
@@ -64,8 +64,9 @@ class Disk : public StorageDevice, private ScsiBlockCommands
 	virtual int Read(span<uint8_t> , 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<PbStatistics> GetStatistics() const override;
@@ -119,7 +120,6 @@ class Disk : public StorageDevice, private ScsiBlockCommands
 	void AddCachePage(map<int, vector<byte>>&, bool) const;
 
 	unordered_set<uint32_t> GetSectorSizes() const;
-	void SetSectorSizes(const unordered_set<uint32_t>& 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
@@ -21,11 +21,8 @@
 using namespace scsi_defs;
 using namespace scsi_command_util;
 
-SCSICD::SCSICD(int lun, const unordered_set<uint32_t>& 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
@@ -25,7 +25,7 @@ class SCSICD : public Disk, private ScsiMmcCommands
 {
 public:
 
-	SCSICD(int, const unordered_set<uint32_t>&, 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
@@ -19,11 +19,9 @@
 
 using namespace scsi_command_util;
 
-SCSIHD::SCSIHD(int lun, const unordered_set<uint32_t>& 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<uint32_t>& 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
@@ -28,7 +28,7 @@ class SCSIHD : public Disk
 
 public:
 
-	SCSIHD(int, const unordered_set<uint32_t>&, bool, scsi_defs::scsi_level = scsi_level::scsi_2);
+	SCSIHD(int, bool, scsi_defs::scsi_level, const unordered_set<uint32_t>& = { 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
@@ -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
@@ -19,10 +19,8 @@
 
 using namespace scsi_command_util;
 
-SCSIMO::SCSIMO(int lun, const unordered_set<uint32_t>& 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
@@ -26,7 +26,7 @@ class SCSIMO : public Disk
 {
 public:
 
-	SCSIMO(int, const unordered_set<uint32_t>&);
+	explicit SCSIMO(int);
 	~SCSIMO() override = default;
 
 	void Open() override;

diff --git 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
@@ -67,7 +67,7 @@ class Piscsi
 
 	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
@@ -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<Pri
 	if (size) {
 		const auto disk = dynamic_pointer_cast<Disk>(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
@@ -11,6 +11,7 @@
 
 #include "hal/bus.h"
 #include "controllers/controller_manager.h"
+#include "devices/device_factory.h"
 #include <unordered_set>
 
 class DeviceFactory;
@@ -60,7 +61,7 @@ class PiscsiExecutor
 
 	ControllerManager& controller_manager;
 
-	const DeviceFactory device_factory;
+	[[no_unique_address]] const DeviceFactory device_factory;
 
 	unordered_set<int> reserved_ids;
 };