Update error handling and logging

cpp/base/primary_device.cpp

@@ -12,6 +12,7 @@
 #include "primary_device.h"
 
 using namespace memory_util;
+using namespace s2p_util;
 
 bool PrimaryDevice::Init(const param_map &params)
 {
@@ -229,20 +230,17 @@ vector<byte> PrimaryDevice::HandleRequestSense() const
         throw scsi_exception(sense_key::not_ready, asc::medium_not_present);
     }
 
-    // Set 18 bytes including extended sense data
-
+    // 18 bytes including extended sense data
     vector<byte> buf(18);
 
     // Current error
     buf[0] = (byte)0x70;
 
-    buf[2] = (byte)(sense_key);
-    buf[7] = (byte)10;
-    buf[12] = (byte)(asc);
+    buf[2] = (byte)sense_key;
+    buf[7] = byte { 10 };
+    buf[12] = (byte)asc;
 
-    LogTrace(
-        fmt::format("Status ${0:02x}, Sense Key ${1:02x}, ASC ${2:02x}", static_cast<int>(GetController()->GetStatus()),
-            static_cast<int>(buf[2]), static_cast<int>(buf[12])));
+    LogTrace(fmt::format("{0}: {1}", STATUS_MAPPING.at(GetController()->GetStatus()), FormatSenseData(sense_key, asc)));
 
     return buf;
 }
@@ -251,31 +249,17 @@ void PrimaryDevice::ReserveUnit()
 {
     reserving_initiator = GetController()->GetInitiatorId();
 
-    if (reserving_initiator != -1) {
-        LogTrace(fmt::format("Reserved device for initiator ID {}", reserving_initiator));
-    }
-    else {
-        LogTrace("Reserved device for unknown initiator");
-    }
-
     StatusPhase();
 }
 
 void PrimaryDevice::ReleaseUnit()
 {
-    if (reserving_initiator != -1) {
-        LogTrace(fmt::format("Released device reserved by initiator ID {}", reserving_initiator));
-    }
-    else {
-        LogTrace("Released device reserved by unknown initiator");
-    }
-
     DiscardReservation();
 
     StatusPhase();
 }
 
-bool PrimaryDevice::CheckReservation(int initiator_id, scsi_command cmd, bool prevent_removal) const
+bool PrimaryDevice::CheckReservation(int initiator_id, scsi_command cmd) const
 {
     if (reserving_initiator == NOT_RESERVED || reserving_initiator == initiator_id) {
         return true;
@@ -288,7 +272,7 @@ bool PrimaryDevice::CheckReservation(int initiator_id, scsi_command cmd, bool pr
     }
 
     // PREVENT ALLOW MEDIUM REMOVAL is permitted if the prevent bit is 0
-    if (cmd == scsi_command::cmd_prevent_allow_medium_removal && !prevent_removal) {
+    if (cmd == scsi_command::cmd_prevent_allow_medium_removal && !(GetController()->GetCdbByte(4) & 0x01)) {
         return true;
     }
 

cpp/base/primary_device.h

@@ -62,7 +62,7 @@ class PrimaryDevice : private ScsiPrimaryCommands, public Device
         return delay_after_bytes;
     }
 
-    bool CheckReservation(int, scsi_command, bool) const;
+    bool CheckReservation(int, scsi_command) const;
     void DiscardReservation();
 
     void Reset() override;

cpp/buses/bus.cpp

@@ -9,6 +9,7 @@
 //---------------------------------------------------------------------------
 
 #include <chrono>
+#include <spdlog/spdlog.h>
 #include "bus_factory.h"
 
 using namespace scsi_defs;
@@ -121,7 +122,7 @@ int Bus::MsgInHandShake()
     WaitBusSettle();
     EnableIRQ();
 
-    const uint8_t msg = GetDAT();
+    const int msg = GetDAT();
 
     SetACK(true);
 
@@ -202,20 +203,26 @@ int Bus::ReceiveHandShake(uint8_t *buf, int count)
 }
 
 // Handshake for DATA OUT and MESSAGE OUT
+#ifdef BUILD_SCDP
 int Bus::SendHandShake(const uint8_t *buf, int count, int daynaport_delay_after_bytes)
+#else
+int Bus::SendHandShake(const uint8_t *buf, int count, int)
+#endif
 {
     int bytes_sent;
 
     DisableIRQ();
 
     if (target_mode) {
         for (bytes_sent = 0; bytes_sent < count; bytes_sent++) {
+#ifdef BUILD_SCDP
             if (bytes_sent == daynaport_delay_after_bytes) {
+                const timespec ts = { .tv_sec = 0, .tv_nsec = SCSI_DELAY_SEND_DATA_DAYNAPORT_NS };
                 EnableIRQ();
-                constexpr timespec ts = { .tv_sec = 0, .tv_nsec = SCSI_DELAY_SEND_DATA_DAYNAPORT_NS };
                 nanosleep(&ts, nullptr);
                 DisableIRQ();
             }
+#endif
 
             SetDAT(*buf);
 
@@ -292,12 +299,14 @@ bool Bus::WaitSignal(int pin, bool state)
             return true;
         }
 
-        // Abort on a reset
         if (GetRST()) {
+            spdlog::warn("Received RST signal during {} phase, aborting", GetPhaseName(GetPhase()));
             return false;
         }
     } while ((chrono::duration_cast < chrono::seconds > (chrono::steady_clock::now() - now).count()) < 3);
 
+    spdlog::trace("Timeout while waiting for ACK/REQ to change to {}", state ? "true" : "false");
+
     return false;
 }
 

cpp/buses/bus.h

@@ -192,7 +192,7 @@ class Bus
         SetSignal(PIN_ACK, state);
     }
 
-    bool GetRST() const
+    inline bool GetRST() const
     {
         return GetSignal(PIN_RST);
     }

cpp/buses/in_process_bus.cpp

@@ -62,7 +62,7 @@ void InProcessBus::SetSignal(int pin, bool state)
 bool InProcessBus::WaitForSelection()
 {
     // Busy waiting cannot be avoided
-    constexpr timespec ts = { .tv_sec = 0, .tv_nsec = 10'000'000 };
+    const timespec ts = { .tv_sec = 0, .tv_nsec = 10'000'000 };
     nanosleep(&ts, nullptr);
 
     return true;

cpp/controllers/generic_controller.cpp

@@ -101,7 +101,7 @@ void GenericController::Command()
         }
 
         const int command_bytes_count = BusFactory::Instance().GetCommandBytesCount(static_cast<scsi_command>(buf[0]));
-        assert(command_bytes_count <= 16);
+        assert(command_bytes_count && command_bytes_count <= 16);
 
         for (int i = 0; i < command_bytes_count; i++) {
             SetCdbByte(i, buf[i]);
@@ -151,7 +151,7 @@ void GenericController::Execute()
         device->SetStatus(sense_key::no_sense, asc::no_additional_sense_information);
     }
 
-    if (device->CheckReservation(GetInitiatorId(), opcode, GetCdbByte(4) & 0x01)) {
+    if (device->CheckReservation(GetInitiatorId(), opcode)) {
         try {
             device->Dispatch(opcode);
 
@@ -172,13 +172,8 @@ void GenericController::Execute()
 void GenericController::Status()
 {
     if (!IsStatus()) {
-        if (const auto &it_status = STATUS_MAPPING.find(GetStatus()); it_status != STATUS_MAPPING.end()) {
-            LogTrace(fmt::format("Status phase, status is {0} (status code ${1:02x})", it_status->second,
+        LogTrace(fmt::format("Status phase, status is {0} (status code ${1:02x})", STATUS_MAPPING.at(GetStatus()),
                 static_cast<int>(GetStatus())));
-        }
-        else {
-            LogTrace(fmt::format("Status phase, status code is ${:02x}", static_cast<int>(GetStatus())));
-        }
 
         SetPhase(phase_t::status);
 
@@ -363,7 +358,7 @@ void GenericController::Receive()
         }
         // Assume that data less than < 256 bytes in DATA OUT are parameters to a non block-oriented command
         else if (IsDataOut() && !GetOffset() && l < 256 && get_level() == level::trace) {
-            LogTrace(fmt::format("{} byte(s) of command parameter data:\n{}", l, FormatBytes(GetBuffer(), l)));
+            LogTrace(fmt::format("{0} byte(s) of command parameter data:\n{1}", l, FormatBytes(GetBuffer(), l)));
         }
     }
 
@@ -410,7 +405,6 @@ void GenericController::Receive()
 
     default:
         error("Unexpected bus phase: " + Bus::GetPhaseName(GetPhase()));
-        assert(false);
         break;
     }
 }

cpp/initiator/initiator_executor.cpp

@@ -28,8 +28,7 @@ int InitiatorExecutor::Execute(scsi_command cmd, span<uint8_t> cdb, span<uint8_t
         warn("CDB has {0} byte(s), command ${1:02x} requires {2} bytes", cdb.size(), static_cast<int>(cmd), count);
     }
 
-    if (const string &command_name = BusFactory::Instance().GetCommandName(cmd);
-    !command_name.empty()) {
+    if (const string &command_name = BusFactory::Instance().GetCommandName(cmd); !command_name.empty()) {
         trace("Executing command {0} for device {1}:{2}", command_name, target_id, target_lun);
     }
     else {
@@ -202,7 +201,7 @@ void InitiatorExecutor::Status()
 {
     array<uint8_t, 1> buf;
 
-    if (bus.ReceiveHandShake(buf.data(), 1) != static_cast<int>(buf.size())) {
+    if (bus.ReceiveHandShake(buf.data(), static_cast<int>(buf.size())) != static_cast<int>(buf.size())) {
         error("STATUS phase failed");
     }
     else {

cpp/protobuf/protobuf_util.cpp

@@ -34,9 +34,7 @@ PbCachingMode protobuf_util::ParseCachingMode(const string &value)
     string v = value;
     ranges::replace(v, '-', '_');
 
-    string m;
-    ranges::transform(v, back_inserter(m), ::toupper);
-    if (PbCachingMode mode; PbCachingMode_Parse(m, &mode)) {
+    if (PbCachingMode mode; PbCachingMode_Parse(ToUpper(v), &mode)) {
         return mode;
     }
 

cpp/s2pctl/sp2ctl_core.cpp

@@ -351,7 +351,7 @@ int S2pCtl::ParseArguments(const vector<char*> &args) // NOSONAR Acceptable comp
                 device->set_caching_mode(ParseCachingMode(optarg));
             }
             catch (const parser_exception &e) {
-                cerr << e.what() << endl;
+                cerr << "Error: " << e.what() << endl;
                 return EXIT_FAILURE;
             }
             break;
@@ -446,7 +446,7 @@ int S2pCtl::ParseArguments(const vector<char*> &args) // NOSONAR Acceptable comp
 
     S2pCtlCommands s2pctl_commands(command, hostname, port, filename_binary, filename_json, filename_text);
 
-    bool status;
+    bool status = false;
     try {
         // Listing devices is a special case
         if (command.operation() == DEVICES_INFO) {
@@ -462,10 +462,7 @@ int S2pCtl::ParseArguments(const vector<char*> &args) // NOSONAR Acceptable comp
     }
     catch (const io_exception &e) {
         cerr << "Error: " << e.what() << endl;
-
-        status = false;
-
-        // Fall through
+        return EXIT_FAILURE;
     }
 
     return status ? EXIT_SUCCESS : EXIT_FAILURE;

cpp/test/primary_device_test.cpp

@@ -86,38 +86,39 @@ TEST(PrimaryDeviceTest, Reset)
     auto [controller, device] = CreatePrimaryDevice();
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_reserve6));
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for initiator ID 1";
     device->Reset();
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved anymore for initiator ID 1";
 }
 
 TEST(PrimaryDeviceTest, CheckReservation)
 {
     auto [controller, device] = CreatePrimaryDevice();
 
-    EXPECT_TRUE(device->CheckReservation(0, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(0, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved for initiator ID 0";
 
     controller->SetInitiatorId(0);
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_reserve6));
-    EXPECT_TRUE(device->CheckReservation(0, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(0, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved for initiator ID 0";
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for initiator ID 1";
-    EXPECT_FALSE(device->CheckReservation(-1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(-1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for unknown initiator";
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_inquiry, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_inquiry))
         << "Device must not be reserved for INQUIRY";
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_request_sense, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_request_sense))
         << "Device must not be reserved for REQUEST SENSE";
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_release6, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_release6))
         << "Device must not be reserved for RELEASE (6)";
 
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_prevent_allow_medium_removal, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_prevent_allow_medium_removal ))
         << "Device must not be reserved for PREVENT ALLOW MEDIUM REMOVAL with prevent bit not set";
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_prevent_allow_medium_removal, true))
+    controller->SetCdbByte(4, 0x01);
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_prevent_allow_medium_removal))
         << "Device must be reserved for PREVENT ALLOW MEDIUM REMOVAL with prevent bit set";
 }
 
@@ -126,19 +127,19 @@ TEST(PrimaryDeviceTest, ReserveReleaseUnit)
     auto [controller, device] = CreatePrimaryDevice();
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_reserve6));
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for initiator ID 1";
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_release6));
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved anymore for initiator ID 1";
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_reserve6));
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for unknown initiator";
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_release6));
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved anymore for unknown initiator";
 }
 
@@ -147,10 +148,10 @@ TEST(PrimaryDeviceTest, DiscardReservation)
     auto [controller, device] = CreatePrimaryDevice();
 
     EXPECT_NO_THROW(device->Dispatch(scsi_command::cmd_reserve6));
-    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_FALSE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must be reserved for initiator ID 1";
     EXPECT_NO_THROW(device->DiscardReservation());
-    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready, false))
+    EXPECT_TRUE(device->CheckReservation(1, scsi_command::cmd_test_unit_ready))
         << "Device must not be reserved anymore for initiator ID 1";
 }