From 1ad7145aed23c281ac91a3b33087f4f88e98a17f Mon Sep 17 00:00:00 2001 From: Uwe Seimet Date: Thu, 2 Jan 2025 17:40:33 +0100 Subject: [PATCH] Improve SCSG LUN and MODE SELECT handling --- cpp/base/device.cpp | 12 +++--- cpp/base/device.h | 7 ++-- cpp/base/primary_device.cpp | 2 + cpp/controllers/controller.cpp | 4 +- cpp/devices/daynaport.cpp | 2 +- cpp/devices/daynaport.h | 2 +- cpp/devices/scsi_generic.cpp | 63 +++++++++++----------------- cpp/devices/scsi_generic.h | 4 +- cpp/initiator/initiator_executor.cpp | 12 +++--- cpp/initiator/initiator_executor.h | 2 +- cpp/s2pexec/s2pexec_core.cpp | 2 +- cpp/shared/command_meta_data.cpp | 4 ++ cpp/shared/scsi.h | 4 ++ cpp/test/command_response_test.cpp | 1 - cpp/test/mocks.h | 9 ---- cpp/test/scsi_generic_test.cpp | 3 -- 16 files changed, 57 insertions(+), 76 deletions(-) diff --git a/cpp/base/device.cpp b/cpp/base/device.cpp index b0cf6af3..d27c5a19 100644 --- a/cpp/base/device.cpp +++ b/cpp/base/device.cpp @@ -37,7 +37,7 @@ void Device::SetParams(const param_map &set_params) params[key] = value; } else { - GetLogger().warn("{0} ignored unknown parameter '{1}={2}'", PbDeviceType_Name(type), key, value); + device_logger->warn("{0} ignored unknown parameter '{1}={2}'", PbDeviceType_Name(type), key, value); } } } @@ -81,13 +81,13 @@ bool Device::Eject(bool force) return true; } -logger& Device::GetLogger() +void Device::CreateLogger() { - if (!logger_initialized) { - device_logger = s2p_util::CreateLogger(fmt::format("[s2p] (ID:LUN {0}:{1})", GetId(), GetLun())); - logger_initialized = true; - } + device_logger = s2p_util::CreateLogger(fmt::format("[s2p] (ID:LUN {0}:{1})", GetId(), GetLun())); +} +logger& Device::GetLogger() const +{ return *device_logger; } diff --git a/cpp/base/device.h b/cpp/base/device.h index 51d14e04..8ce7628c 100644 --- a/cpp/base/device.h +++ b/cpp/base/device.h @@ -118,7 +118,7 @@ class Device // NOSONAR The number of fields and methods is justified, the compl void Stop(); virtual bool Eject(bool); - logger& GetLogger(); + logger& GetLogger() const; protected: @@ -162,6 +162,7 @@ class Device // NOSONAR The number of fields and methods is justified, the compl string GetParam(const string&) const; + void CreateLogger(); void LogTrace(const string&) const; void LogDebug(const string&) const; void LogWarn(const string&) const; @@ -193,11 +194,9 @@ class Device // NOSONAR The number of fields and methods is justified, the compl bool supports_params = false; - bool logger_initialized = false; - // The parameters the device was created with param_map params; - // Use the default logger until the device-specific logger has been initialized + // Use the default logger until the device-specific logger has been created shared_ptr device_logger = default_logger(); }; diff --git a/cpp/base/primary_device.cpp b/cpp/base/primary_device.cpp index 85a75195..f7971ef4 100644 --- a/cpp/base/primary_device.cpp +++ b/cpp/base/primary_device.cpp @@ -186,6 +186,8 @@ bool PrimaryDevice::SetResponseDataFormat(ScsiLevel l) void PrimaryDevice::SetController(AbstractController *c) { controller = c; + + CreateLogger(); } void PrimaryDevice::StatusPhase() const diff --git a/cpp/controllers/controller.cpp b/cpp/controllers/controller.cpp index fc946705..29fa5fba 100644 --- a/cpp/controllers/controller.cpp +++ b/cpp/controllers/controller.cpp @@ -508,8 +508,8 @@ bool Controller::TransferFromHost(int length) int transferred_length = length; const auto device = GetDeviceForLun(GetEffectiveLun()); try { - // TODO Try to remove this special case - if (cmd == ScsiCommand::MODE_SELECT_6 || cmd == ScsiCommand::MODE_SELECT_10) { + // TODO Try to remove these special cases (MODE SELECT case and SCSG case) + if ((cmd == ScsiCommand::MODE_SELECT_6 || cmd == ScsiCommand::MODE_SELECT_10) && device->GetType() != SCSG) { // The offset is the number of bytes transferred, i.e. the length of the parameter list device->ModeSelect(GetCdb(), GetBuffer(), GetOffset(), 0); } diff --git a/cpp/devices/daynaport.cpp b/cpp/devices/daynaport.cpp index 85c1b151..1c7cafbb 100644 --- a/cpp/devices/daynaport.cpp +++ b/cpp/devices/daynaport.cpp @@ -365,7 +365,7 @@ void DaynaPort::SetMcastAddr() const // seconds // //--------------------------------------------------------------------------- -void DaynaPort::EnableInterface() +void DaynaPort::EnableInterface() const { if (GetCdbByte(5) & 0x80) { if (const string &error = TapDriver::IpLink(true, GetLogger()); !error.empty()) { diff --git a/cpp/devices/daynaport.h b/cpp/devices/daynaport.h index 3ac8689d..e33a6f48 100644 --- a/cpp/devices/daynaport.h +++ b/cpp/devices/daynaport.h @@ -57,7 +57,7 @@ class DaynaPort : public PrimaryDevice void RetrieveStats() const; void SetInterfaceMode() const; void SetMcastAddr() const; - void EnableInterface(); + void EnableInterface() const; vector GetStatistics() const override; diff --git a/cpp/devices/scsi_generic.cpp b/cpp/devices/scsi_generic.cpp index 4ea23bce..2a27ac72 100644 --- a/cpp/devices/scsi_generic.cpp +++ b/cpp/devices/scsi_generic.cpp @@ -96,28 +96,13 @@ void ScsiGeneric::Dispatch(ScsiCommand cmd) remaining_count = byte_count; - auto &buf = GetController()->GetBuffer(); - // There is no explicit LUN support, the SG driver maps each LUN to a device file - if ((static_cast(local_cdb[1]) & 0b11100000) && cmd != ScsiCommand::INQUIRY) { - if (cmd != ScsiCommand::REQUEST_SENSE) { - throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::LOGICAL_UNIT_NOT_SUPPORTED); - } - - fill_n(buf.begin(), 18, 0); - buf[0] = 0x70; - buf[2] = static_cast(SenseKey::ILLEGAL_REQUEST); - buf[7] = 10; - buf[12] = static_cast(Asc::LOGICAL_UNIT_NOT_SUPPORTED); - - const int length = min(18, byte_count); - GetController()->SetTransferSize(length, length); - DataInPhase(length); - - // When signalling an invalid LUN, for REQUEST SENSE the status must be GOOD - return; + if (GetController()->GetEffectiveLun() && cmd != ScsiCommand::INQUIRY) { + throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::LOGICAL_UNIT_NOT_SUPPORTED); } + auto &buf = GetController()->GetBuffer(); + if (cmd == ScsiCommand::REQUEST_SENSE && deferred_sense_data_valid) { memcpy(buf.data(), deferred_sense_data.data(), deferred_sense_data.size()); deferred_sense_data_valid = false; @@ -150,12 +135,13 @@ void ScsiGeneric::Dispatch(ScsiCommand cmd) vector ScsiGeneric::InquiryInternal() const { - return HandleInquiry(DeviceType::OPTICAL_MEMORY, true); + assert(false); + return {}; } int ScsiGeneric::ReadData(data_in_t buf) { - return ReadWriteData(buf, GetController()->GetChunkSize(), true); + return ReadWriteData(buf, GetController()->GetChunkSize()); } int ScsiGeneric::WriteData(cdb_t, data_out_t buf, int, int length) @@ -181,10 +167,10 @@ int ScsiGeneric::WriteData(cdb_t, data_out_t buf, int, int length) } } - return ReadWriteData(span((uint8_t*)buf.data(), buf.size()), length, true); // NOSONAR Cast required for SG driver API + return ReadWriteData(span((uint8_t*)buf.data(), buf.size()), length); // NOSONAR Cast required for SG driver API } -int ScsiGeneric::ReadWriteData(span buf, int chunk_size, bool enable_log) +int ScsiGeneric::ReadWriteData(span buf, int chunk_size) { int length = remaining_count < chunk_size ? remaining_count : chunk_size; length = length < MAX_TRANSFER_LENGTH ? length : MAX_TRANSFER_LENGTH; @@ -218,26 +204,26 @@ int ScsiGeneric::ReadWriteData(span buf, int chunk_size, bool enable_lo TIMEOUT_FORMAT_SECONDS : TIMEOUT_DEFAULT_SECONDS) * 1000; // Check the log level in order to avoid an unnecessary time-consuming string construction - if (enable_log && GetLogger().level() <= level::debug) { + if (GetController() && GetLogger().level() <= level::debug) { LogDebug(command_meta_data.LogCdb(local_cdb, "SG driver")); } - if (enable_log && write && GetLogger().level() == level::trace) { - LogTrace(fmt::format("Transferring {0} byte(s) to SG driver:\n{1}", length, - GetController()->FormatBytes(buf, length))); + if (write && GetController() && GetLogger().level() == level::trace) { + LogTrace(fmt::format("Transferring {0} byte(s) to SG driver{1}", length, + length ? fmt::format(":\n{}", GetController()->FormatBytes(buf, length)) : "")); } const int status = ioctl(fd, SG_IO, &io_hdr) < 0 ? -1 : io_hdr.status; format_header.clear(); - EvaluateStatus(status, span(buf.data(), length), sense_data, write, enable_log); + EvaluateStatus(status, span(buf.data(), length), sense_data, write); const int transferred_length = length - io_hdr.resid; - if (enable_log && !write && GetLogger().level() == level::trace) { - LogTrace(fmt::format("Transferred {0} byte(s) from SG driver:\n{1}", transferred_length, - GetController()->FormatBytes(buf, transferred_length))); + if (!write && GetController() && GetLogger().level() == level::trace) { + LogTrace(fmt::format("Transferred {0} byte(s) from SG driver{1}", transferred_length, + transferred_length ? fmt::format(":\n{}", GetController()->FormatBytes(buf, transferred_length)) : "")); } UpdateInternalBlockSize(buf, length); @@ -252,17 +238,17 @@ int ScsiGeneric::ReadWriteData(span buf, int chunk_size, bool enable_lo remaining_count = 0; } - if (enable_log) { + if (GetController()) { LogTrace(fmt::format("{0} byte(s) transferred, {1} byte(s) remaining", transferred_length, remaining_count)); } return transferred_length; } -void ScsiGeneric::EvaluateStatus(int status, span buf, span sense_data, bool write, bool enable_log) +void ScsiGeneric::EvaluateStatus(int status, span buf, span sense_data, bool write) { if (status == -1) { - if (enable_log) { + if (GetController()) { LogError(fmt::format("Transfer of {0} byte(s) failed: {1}", buf.size(), strerror(errno))); } @@ -276,8 +262,8 @@ void ScsiGeneric::EvaluateStatus(int status, span buf, span se if (!status) { status = static_cast(sense_data[2]) & 0x0f; - if (static_cast(local_cdb[0]) == ScsiCommand::INQUIRY - && (static_cast(local_cdb[1]) & 0b11100000)) { + if (static_cast(local_cdb[0]) == ScsiCommand::INQUIRY && GetController() + && GetController()->GetEffectiveLun()) { // SCSI-2 section 8.2.5.1: Incorrect logical unit handling buf[0] = 0x7f; } @@ -304,7 +290,6 @@ void ScsiGeneric::UpdateInternalBlockSize(span buf, int length) if (block_size != size) { LogTrace(fmt::format("Updating internal block size to {} bytes", size)); - assert(size); if (size) { block_size = size; } @@ -324,7 +309,7 @@ string ScsiGeneric::GetDeviceData() local_cdb[4] = static_cast(byte_count); try { - ReadWriteData(buf, byte_count, false); + ReadWriteData(buf, byte_count); } catch (const ScsiException &e) { return e.what(); @@ -352,7 +337,7 @@ void ScsiGeneric::GetBlockSize() try { // Trigger a block size update - ReadWriteData(buf, byte_count, false); + ReadWriteData(buf, byte_count); } catch (const ScsiException&) { // NOSONAR The exception details do not matter, this might not be a block device // Fall through diff --git a/cpp/devices/scsi_generic.h b/cpp/devices/scsi_generic.h index 5f956afd..b5feb0dd 100644 --- a/cpp/devices/scsi_generic.h +++ b/cpp/devices/scsi_generic.h @@ -46,9 +46,9 @@ class ScsiGeneric : public PrimaryDevice private: - int ReadWriteData(span, int, bool); + int ReadWriteData(span, int); - void EvaluateStatus(int, span, span, bool, bool); + void EvaluateStatus(int, span, span, bool); void UpdateInternalBlockSize(span buf, int); diff --git a/cpp/initiator/initiator_executor.cpp b/cpp/initiator/initiator_executor.cpp index 14d1f250..d8dcdbeb 100644 --- a/cpp/initiator/initiator_executor.cpp +++ b/cpp/initiator/initiator_executor.cpp @@ -52,7 +52,7 @@ int InitiatorExecutor::Execute(span cdb, span buffer, int leng return 0xff; } - if (!Selection()) { + if (!Selection(cdb[1] & 0b11100000)) { bus.Reset(); return 0xff; } @@ -163,7 +163,7 @@ bool InitiatorExecutor::Arbitration() const return true; } -bool InitiatorExecutor::Selection() const +bool InitiatorExecutor::Selection(bool explicit_lun) const { initiator_logger.trace("Selection of target {0} with initiator ID {1}", target_id, initiator_id); @@ -172,17 +172,17 @@ bool InitiatorExecutor::Selection() const bus.SetSEL(true); - if (!sasi) { + if (!sasi && !explicit_lun) { // Request MESSAGE OUT for IDENTIFY bus.SetATN(true); Sleep(DESKEW_DELAY); Sleep(DESKEW_DELAY); + } - bus.SetBSY(false); + bus.SetBSY(false); - Sleep(BUS_SETTLE_DELAY); - } + Sleep(BUS_SETTLE_DELAY); if (!WaitForBusy()) { initiator_logger.trace("Selection failed"); diff --git a/cpp/initiator/initiator_executor.h b/cpp/initiator/initiator_executor.h index 5df32743..a9e8dd54 100644 --- a/cpp/initiator/initiator_executor.h +++ b/cpp/initiator/initiator_executor.h @@ -61,7 +61,7 @@ class InitiatorExecutor bool Dispatch(span, span, int&); bool Arbitration() const; - bool Selection() const; + bool Selection(bool) const; void Command(span); void Status(); void DataIn(data_in_t, int&); diff --git a/cpp/s2pexec/s2pexec_core.cpp b/cpp/s2pexec/s2pexec_core.cpp index dc0e3fa3..b5fe144a 100644 --- a/cpp/s2pexec/s2pexec_core.cpp +++ b/cpp/s2pexec/s2pexec_core.cpp @@ -330,8 +330,8 @@ bool S2pExec::ParseArguments(span args, bool in_process) if (buffer_size = ParseAsUnsignedInt(buf); buffer_size <= 0) { throw ParserException("Invalid receive buffer size: '" + buf + "'"); } - buffer.resize(buffer_size); } + buffer.resize(buffer_size); return true; } diff --git a/cpp/shared/command_meta_data.cpp b/cpp/shared/command_meta_data.cpp index 9f7501c9..af281b65 100644 --- a/cpp/shared/command_meta_data.cpp +++ b/cpp/shared/command_meta_data.cpp @@ -68,6 +68,8 @@ CommandMetaData::CommandMetaData() AddCommand(ScsiCommand::VERIFY_10, 10, "VERIFY(10)", { 7, 2, 2, 4, true }); AddCommand(ScsiCommand::SYNCHRONIZE_CACHE_10, 10, "SYNCHRONIZE CACHE(10)", { 0, 0, 0, 0, false }); AddCommand(ScsiCommand::READ_DEFECT_DATA_10, 10, "READ DEFECT DATA(10)", { 7, 2, 0, 0, false }); + AddCommand(ScsiCommand::WRITE_BUFFER, 10, "READ BUFFER", { 6, 3, 0, 0, true }); + AddCommand(ScsiCommand::READ_BUFFER_10, 10, "READ BUFFER(10)", { 6, 3, 0, 0, false }); AddCommand(ScsiCommand::READ_LONG_10, 10, "READ LONG(10)", { 7, 2, 0, 0, false }); AddCommand(ScsiCommand::WRITE_LONG_10, 10, "WRITE LONG(10)", { 7, 2, 0, 0, true }); AddCommand(ScsiCommand::WRITE_SAME_10, 10, "WRITE SAME(10)", { 7, 2, 0, 0, true }); @@ -90,6 +92,7 @@ CommandMetaData::CommandMetaData() AddCommand(ScsiCommand::READ_MASTER_CUE, 10, "READ MASTER CUE", { 6, 3, 0, 0, false }); AddCommand(ScsiCommand::MODE_SENSE_10, 10, "MODE SENSE(10)", { 7, 2, 0, 0, false }); AddCommand(ScsiCommand::CLOSE_TRACK_SESSION, 10, "CLOSE TRACK/SESSION", { 0, 0, 0, 0, false }); + AddCommand(ScsiCommand::READ_BUFFER_CAPACITY, 10, "READ BUFFER CAPACITY", { 7, 2, 0, 0, false }); AddCommand(ScsiCommand::PERSISTENT_RESERVE_IN, 10, "PERSISTENT RESERVE IN", { 7, 2, 0, 0, false }); AddCommand(ScsiCommand::PERSISTENT_RESERVE_OUT, 10, "PERSISTENT RESERVE OUT", { 7, 2, 0, 0, true }); AddCommand(ScsiCommand::WRITE_FILEMARKS_16, 16, "WRITE FILEMARKS(16)", { 0, 0, 0, 0, false }); @@ -101,6 +104,7 @@ CommandMetaData::CommandMetaData() AddCommand(ScsiCommand::READ_POSITION, 10, "READ POSITION", { 7, 2, 0, 0, false }); AddCommand(ScsiCommand::SYNCHRONIZE_CACHE_SPACE_16, 16, "SYNCHRONIZE CACHE(16)/SPACE(16)", { 0, 0, 0, 0, false }); AddCommand(ScsiCommand::LOCATE_16, 16, "LOCATE(16)", { 0, 0, 0, 0, false }); + AddCommand(ScsiCommand::READ_BUFFER_16, 16, "READ BUFFER(16)", { 10, 4, 0, 0, false }); AddCommand(ScsiCommand::ERASE_WRITE_SAME_16, 16, "ERASE(16)/WRITE SAME(16)", { 0, 0, 0, 0, false }); AddCommand(ScsiCommand::READ_CAPACITY_READ_LONG_16, 16, "READ CAPACITY(16)/READ LONG(16)", { 12, 2, 0, 0, false }); diff --git a/cpp/shared/scsi.h b/cpp/shared/scsi.h index fb5b9d01..d7367fb0 100644 --- a/cpp/shared/scsi.h +++ b/cpp/shared/scsi.h @@ -98,6 +98,8 @@ enum class ScsiCommand READ_POSITION = 0x34, SYNCHRONIZE_CACHE_10 = 0x35, READ_DEFECT_DATA_10 = 0x37, + WRITE_BUFFER = 0x3b, + READ_BUFFER_10 = 0x3c, READ_LONG_10 = 0x3e, WRITE_LONG_10 = 0x3f, WRITE_SAME_10 = 0x41, @@ -120,6 +122,7 @@ enum class ScsiCommand READ_MASTER_CUE = 0x59, MODE_SENSE_10 = 0x5a, CLOSE_TRACK_SESSION = 0x5b, + READ_BUFFER_CAPACITY = 0x5c, PERSISTENT_RESERVE_IN = 0x5e, PERSISTENT_RESERVE_OUT = 0x5f, WRITE_FILEMARKS_16 = 0x80, @@ -131,6 +134,7 @@ enum class ScsiCommand SYNCHRONIZE_CACHE_SPACE_16 = 0x91, LOCATE_16 = 0x92, ERASE_WRITE_SAME_16 = 0x93, + READ_BUFFER_16 = 0x9b, READ_CAPACITY_READ_LONG_16 = 0x9e, WRITE_LONG_16 = 0x9f, REPORT_LUNS = 0xa0, diff --git a/cpp/test/command_response_test.cpp b/cpp/test/command_response_test.cpp index 255ecc31..708e1176 100644 --- a/cpp/test/command_response_test.cpp +++ b/cpp/test/command_response_test.cpp @@ -34,7 +34,6 @@ void TestNonDiskDevice(PbDeviceType type, unsigned int default_param_count) auto d = DeviceFactory::Instance().CreateDevice(type, 0, ""); const param_map params; - d->GetLogger(); d->Init(); EXPECT_TRUE(controller_factory.AttachToController(bus, 0, d)); diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index bb1e18d3..5e4c3d17 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -294,7 +294,6 @@ class MockPrimaryDevice : public PrimaryDevice explicit MockPrimaryDevice(int lun) : PrimaryDevice(UNDEFINED, lun) { - GetLogger(); } ~MockPrimaryDevice() override = default; @@ -331,7 +330,6 @@ class MockStorageDevice : public StorageDevice MockStorageDevice() : StorageDevice(UNDEFINED, 0, false, false, { 256, 512, 1024, 2048, 4096 }) { - GetLogger(); } ~MockStorageDevice() override = default; @@ -391,7 +389,6 @@ class MockDisk : public Disk MockDisk() : Disk(SCHD, 0, false, false, { 512, 1024, 2048, 4096 }) { - GetLogger(); SetCachingMode(PbCachingMode::LINUX); SetBlockSize(512); } @@ -405,7 +402,6 @@ class MockSasiHd : public SasiHd explicit MockSasiHd(int lun) : SasiHd(lun) { - GetLogger(); SetCachingMode(PbCachingMode::PISCSI); } ~MockSasiHd() override = default; @@ -432,13 +428,11 @@ class MockScsiHd : public ScsiHd MockScsiHd(int lun, bool removable) : ScsiHd(lun, removable, false, false) { - GetLogger(); SetCachingMode(PbCachingMode::PISCSI); } explicit MockScsiHd(const set §or_sizes) : ScsiHd(0, false, false, false, sector_sizes) { - GetLogger(); SetCachingMode(PbCachingMode::PISCSI); } ~MockScsiHd() override = default; @@ -454,7 +448,6 @@ class MockScsiCd : public ScsiCd explicit MockScsiCd(int lun) : ScsiCd(lun, false) { - GetLogger(); SetCachingMode(PbCachingMode::PISCSI); } }; @@ -470,7 +463,6 @@ class MockOpticalMemory : public OpticalMemory explicit MockOpticalMemory(int lun) : OpticalMemory(lun) { - GetLogger(); SetCachingMode(PbCachingMode::PISCSI); } }; @@ -495,7 +487,6 @@ class MockTape : public Tape MockTape() : Tape(0) { - GetLogger(); } void SetReady(bool b) diff --git a/cpp/test/scsi_generic_test.cpp b/cpp/test/scsi_generic_test.cpp index f5dfbc9a..9c81b6b6 100644 --- a/cpp/test/scsi_generic_test.cpp +++ b/cpp/test/scsi_generic_test.cpp @@ -59,15 +59,12 @@ TEST(ScsiGenericTest, SetProductData) TEST(ScsiGenericTest, SetUp) { ScsiGeneric device1(0, ""); - device1.GetLogger(); EXPECT_NE("", device1.SetUp()); ScsiGeneric device2(0, "/dev/null"); - device2.GetLogger(); EXPECT_NE("", device2.SetUp()); ScsiGeneric device3(0, "/dev/sg0123456789"); - device3.GetLogger(); EXPECT_NE("", device3.SetUp()); }