Report NOT READY early, update error handling

uweseimet · Jan 3, 2025 · 9ae966e · 9ae966e
1 parent 8524b59
commit 9ae966e
Show file tree

Hide file tree

Showing 13 changed files with 163 additions and 74 deletions.
diff --git a/cpp/base/primary_device.cpp b/cpp/base/primary_device.cpp
@@ -67,7 +67,6 @@ void PrimaryDevice::Dispatch(ScsiCommand cmd)
         command();
     }
     else {
-        LogTrace(fmt::format("Device received unsupported command: ${:02x}", static_cast<int>(cmd)));
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_COMMAND_OPERATION_CODE);
     }
 }

diff --git a/cpp/controllers/abstract_controller.cpp b/cpp/controllers/abstract_controller.cpp
@@ -151,19 +151,12 @@ ShutdownMode AbstractController::ProcessOnController(int ids)
 bool AbstractController::AddDevice(shared_ptr<PrimaryDevice> device)
 {
     const int lun = device->GetLun();
-    if (lun < 0 || lun >= 32 || GetDeviceForLun(lun) || device->GetController()) {
+    if (lun < 0 || lun >= (device->GetType() == SAHD ? 2 : 32) || luns.contains(lun) || device->GetController()) {
         return false;
     }
 
-    for (const auto& [_, d] : luns) {
-        if ((device->GetType() == SAHD && d->GetType() != SAHD)
-            || (device->GetType() != SAHD && d->GetType() == SAHD)) {
-            controller_logger->error("SCSI and SASI devices cannot share a controller");
-            return false;
-        }
-    }
-
     luns[lun] = device;
+
     device->SetController(this);
 
     return true;

diff --git a/cpp/controllers/controller.cpp b/cpp/controllers/controller.cpp
@@ -534,9 +534,13 @@ void Controller::XferMsg()
     assert(IsMsgOut());
 
     if (atn_msg) {
-        msg_bytes.emplace_back(GetBuffer()[0]);
+        const auto msg = GetBuffer()[0];
+        msg_bytes.emplace_back(msg);
 
-        LogTrace(fmt::format("Received message byte ${:02x}", GetBuffer()[0]));
+        // Do not log IDENTIFY message twice
+        if (msg < 0x80) {
+            LogTrace(fmt::format("Received message byte ${:02x}", msg));
+        }
     }
 }
 

diff --git a/cpp/controllers/controller.h b/cpp/controllers/controller.h
@@ -19,8 +19,7 @@ class Controller : public AbstractController
 
     bool Process() override;
 
-    void Error(SenseKey, Asc asc = Asc::NO_ADDITIONAL_SENSE_INFORMATION, StatusCode status =
-        StatusCode::CHECK_CONDITION) override;
+    void Error(SenseKey, Asc = Asc::NO_ADDITIONAL_SENSE_INFORMATION, StatusCode = StatusCode::CHECK_CONDITION) override;
     void Reset() override;
 
     void BusFree() override;

diff --git a/cpp/devices/disk.cpp b/cpp/devices/disk.cpp
@@ -54,7 +54,8 @@ string Disk::SetUp()
         });
     AddCommand(ScsiCommand::SEEK_6, [this]
         {
-            Seek(SEEK6);
+            CheckAndGetStartAndCount(SEEK6);
+            StatusPhase();
         });
     AddCommand(ScsiCommand::READ_CAPACITY_10, [this]
         {
@@ -82,7 +83,8 @@ string Disk::SetUp()
         });
     AddCommand(ScsiCommand::SEEK_10, [this]
         {
-            Seek(SEEK10);
+            CheckAndGetStartAndCount(SEEK10);
+            StatusPhase();
         });
     AddCommand(ScsiCommand::VERIFY_10, [this]
         {
@@ -429,16 +431,6 @@ int Disk::WriteData(cdb_t cdb, data_out_t buf, int, int l)
     return l;
 }
 
-void Disk::Seek(AccessMode mode)
-{
-    const auto& [valid, start, count] = CheckAndGetStartAndCount(mode);
-    if (valid) {
-        CheckReady();
-    }
-
-    StatusPhase();
-}
-
 void Disk::ReadCapacity10()
 {
     CheckReady();
@@ -513,16 +505,18 @@ void Disk::ReadCapacity16_ReadLong16()
     }
 }
 
-uint64_t Disk::ValidateBlockAddress(AccessMode mode) const
+uint64_t Disk::ValidateBlockAddress(AccessMode mode)
 {
+    CheckReady();
+
     // RelAdr is not supported
     if (mode == RW10 && GetCdbByte(1) & 0x01) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_FIELD_IN_CDB);
     }
 
     const uint64_t sector = mode == RW16 ? GetCdbInt64(2) : GetCdbInt32(2);
 
-    if (sector > GetBlockCount()) {
+    if (sector >= GetBlockCount()) {
         LogTrace(
             fmt::format("Capacity of {0} sector(s) exceeded: Trying to access sector {1}", GetBlockCount(), sector));
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::LBA_OUT_OF_RANGE);
@@ -543,8 +537,11 @@ void Disk::ChangeBlockSize(uint32_t new_size)
     }
 }
 
-tuple<bool, uint64_t, uint32_t> Disk::CheckAndGetStartAndCount(AccessMode mode) const
+tuple<bool, uint64_t, uint32_t> Disk::CheckAndGetStartAndCount(AccessMode mode)
 {
+    spdlog::critical(IsReady());
+    CheckReady();
+
     uint64_t start;
     uint32_t count;
 

diff --git a/cpp/devices/disk.h b/cpp/devices/disk.h
@@ -83,7 +83,6 @@ class Disk : public StorageDevice
     void Read(AccessMode);
     void Write(AccessMode);
     void Verify(AccessMode);
-    void Seek(AccessMode);
     void ReadCapacity16_ReadLong16();
 
     void AddVerifyErrorRecoveryPage(map<int, vector<byte>>&, bool) const;
@@ -93,8 +92,8 @@ class Disk : public StorageDevice
 
     void ReadWriteLong(uint64_t, uint32_t, bool);
     void WriteVerify(uint64_t, uint32_t, bool);
-    uint64_t ValidateBlockAddress(AccessMode) const;
-    tuple<bool, uint64_t, uint32_t> CheckAndGetStartAndCount(AccessMode) const;
+    uint64_t ValidateBlockAddress(AccessMode);
+    tuple<bool, uint64_t, uint32_t> CheckAndGetStartAndCount(AccessMode);
 
     shared_ptr<Cache> cache;
 

diff --git a/cpp/devices/tape.cpp b/cpp/devices/tape.cpp
@@ -117,6 +117,8 @@ void Tape::ValidateFile()
 
 void Tape::Read(bool read_16)
 {
+    CheckReady();
+
     // FIXED and SILI must not both be set, only partition 0 is supported
     if ((GetCdbByte(1) & 0b11) == 0b11 || (read_16 && GetCdbByte(3))) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_FIELD_IN_CDB);
@@ -148,6 +150,8 @@ void Tape::Read(bool read_16)
 
 void Tape::Write(bool write_16)
 {
+    CheckReady();
+
     // FCS and LCS are not supported, only partition 0 is supported
     if (write_16 && (GetCdbByte(1) & 0b1100 || GetCdbByte(3))) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_FIELD_IN_CDB);
@@ -180,8 +184,6 @@ void Tape::Write(bool write_16)
 
 int Tape::ReadData(data_in_t buf)
 {
-    CheckReady();
-
     int length = GetController()->GetChunkSize();
 
     if (IsAtRecordBoundary()) {
@@ -247,8 +249,6 @@ int Tape::ReadData(data_in_t buf)
 
 int Tape::WriteData(cdb_t, data_out_t buf, int, int chunk_size)
 {
-    CheckReady();
-
     if (IsAtRecordBoundary()) {
         WriteMetaData(ObjectType::BLOCK, record_length);
     }
@@ -435,6 +435,8 @@ void Tape::AddMediumPartitionPage(map<int, vector<byte> > &pages, bool changeabl
 
 void Tape::Erase6()
 {
+    CheckReady();
+
     if (tar_file) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_COMMAND_OPERATION_CODE);
     }
@@ -467,6 +469,8 @@ void Tape::ReadBlockLimits() const
 
 void Tape::Space6()
 {
+    CheckReady();
+
     if (tar_file) {
         LogError("In tar-compatibility mode spacing is not supported");
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_COMMAND_OPERATION_CODE);
@@ -494,6 +498,8 @@ void Tape::Space6()
 
 void Tape::WriteFilemarks(bool write_filemarks_16)
 {
+    CheckReady();
+
     if (tar_file) {
         LogTrace("In tar-compatibility mode writing filemarks is ignored");
         StatusPhase();
@@ -532,6 +538,8 @@ void Tape::WriteFilemarks(bool write_filemarks_16)
 
 bool Tape::Locate(bool locate_16)
 {
+    CheckReady();
+
     // CP is not supported
     if (GetCdbByte(1) & 0x02) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_FIELD_IN_CDB);
@@ -569,8 +577,10 @@ bool Tape::Locate(bool locate_16)
     return true;
 }
 
-void Tape::ReadPosition() const
+void Tape::ReadPosition()
 {
+    CheckReady();
+
     auto &buf = GetController()->GetBuffer();
     fill_n(buf.begin(), 20, 0);
 
@@ -596,6 +606,8 @@ void Tape::ReadPosition() const
 
 void Tape::FormatMedium()
 {
+    CheckReady();
+
     if (tar_file) {
         throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_COMMAND_OPERATION_CODE);
     }

diff --git a/cpp/devices/tape.h b/cpp/devices/tape.h
@@ -77,7 +77,7 @@ class Tape : public StorageDevice
     void Space6();
     void WriteFilemarks(bool);
     void FormatMedium();
-    void ReadPosition() const;
+    void ReadPosition();
     bool Locate(bool);
 
     void WriteMetaData(Tape::ObjectType, uint32_t = 0);

diff --git a/cpp/s2ptool/s2ptool.cpp b/cpp/s2ptool/s2ptool.cpp
@@ -106,13 +106,13 @@ int main(int argc, char *argv[])
     vector<char*> client_args;
     add_arg(client_args, client);
     for (const auto &arg : Split(c_args, ' ')) {
-        add_arg(client_args, arg);
+        add_arg(client_args, arg != "''" && arg != "\"\"" ? arg : "");
     }
 
     vector<char*> target_args;
     add_arg(target_args, "s2p");
     for (const auto &arg : Split(t_args, ' ')) {
-        add_arg(target_args, arg);
+        add_arg(target_args, arg != "''" && arg != "\"\"" ? arg : "");
     }
 
 #ifndef __APPLE__

diff --git a/cpp/shared/command_meta_data.h b/cpp/shared/command_meta_data.h
@@ -42,7 +42,7 @@ class CommandMetaData
         return command_byte_counts[static_cast<int>(cmd)];
     }
 
-    auto GetCommandName(ScsiCommand cmd) const
+    const string& GetCommandName(ScsiCommand cmd) const
     {
         return command_names[static_cast<int>(cmd)];
     }

diff --git a/cpp/test/abstract_controller_test.cpp b/cpp/test/abstract_controller_test.cpp
@@ -89,7 +89,9 @@ TEST(AbstractControllerTest, AddDevice)
     MockAbstractController controller;
 
     EXPECT_TRUE(controller.AddDevice(make_shared<MockPrimaryDevice>(0)));
-    EXPECT_TRUE(controller.AddDevice(make_shared<MockScsiHd>(1, false)));
+    EXPECT_TRUE(controller.AddDevice(make_shared<MockScsiHd>(3, false)));
+    EXPECT_FALSE(controller.AddDevice(make_shared<MockScsiHd>(32, false)));
+    EXPECT_TRUE(controller.AddDevice(make_shared<MockSasiHd>(1)));
     EXPECT_FALSE(controller.AddDevice(make_shared<MockSasiHd>(2)));
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -67,7 +67,6 @@ void PrimaryDevice::Dispatch(ScsiCommand cmd) @@
             command();
         }
         else {
-            LogTrace(fmt::format("Device received unsupported command: ${:02x}", static_cast<int>(cmd)));
             throw ScsiException(SenseKey::ILLEGAL_REQUEST, Asc::INVALID_COMMAND_OPERATION_CODE);
         }
     }
@@ Expand Down @@