Improve SCSG LUN and MODE SELECT handling

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;
 }
 

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<logger> device_logger = default_logger();
 };

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

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);
         }

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()) {

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<PbStatistics> GetStatistics() const override;
 

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<int>(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<uint8_t>(SenseKey::ILLEGAL_REQUEST);
-        buf[7] = 10;
-        buf[12] = static_cast<uint8_t>(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<uint8_t> 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<uint8_t> buf, int chunk_size, bool enable_log)
+int ScsiGeneric::ReadWriteData(span<uint8_t> 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<uint8_t> 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<uint8_t> 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<uint8_t> buf, span<uint8_t> sense_data, bool write, bool enable_log)
+void ScsiGeneric::EvaluateStatus(int status, span<uint8_t> buf, span<uint8_t> 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<uint8_t> buf, span<uint8_t> se
     if (!status) {
         status = static_cast<int>(sense_data[2]) & 0x0f;
 
-        if (static_cast<ScsiCommand>(local_cdb[0]) == ScsiCommand::INQUIRY
-            && (static_cast<int>(local_cdb[1]) & 0b11100000)) {
+        if (static_cast<ScsiCommand>(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<uint8_t> 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<uint8_t>(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

cpp/devices/scsi_generic.h

@@ -46,9 +46,9 @@ class ScsiGeneric : public PrimaryDevice
 
 private:
 
-    int ReadWriteData(span<uint8_t>, int, bool);
+    int ReadWriteData(span<uint8_t>, int);
 
-    void EvaluateStatus(int, span<uint8_t>, span<uint8_t>, bool, bool);
+    void EvaluateStatus(int, span<uint8_t>, span<uint8_t>, bool);
 
     void UpdateInternalBlockSize(span<uint8_t> buf, int);
 

cpp/initiator/initiator_executor.cpp

@@ -52,7 +52,7 @@ int InitiatorExecutor::Execute(span<uint8_t> cdb, span<uint8_t> 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");

cpp/initiator/initiator_executor.h

@@ -61,7 +61,7 @@ class InitiatorExecutor
     bool Dispatch(span<uint8_t>, span<uint8_t>, int&);
 
     bool Arbitration() const;
-    bool Selection() const;
+    bool Selection(bool) const;
     void Command(span<uint8_t>);
     void Status();
     void DataIn(data_in_t, int&);

cpp/s2pexec/s2pexec_core.cpp

@@ -330,8 +330,8 @@ bool S2pExec::ParseArguments(span<char*> 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;
 }

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 });

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,

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));