Only delete bridge if s2p created it, improve initiator tools error h…

…andling

cpp/Makefile

@@ -116,9 +116,9 @@ ifeq ($(BOARD), FULLSPEC)
 endif
 
 DIR_SHARED := shared
-DIR_SHARED_PROTOBUF := shared_protobuf
-DIR_SHARED_COMMAND := shared_command
-DIR_SHARED_INITIATOR := shared_initiator
+DIR_SHARED_PROTOBUF := protobuf
+DIR_SHARED_COMMAND := command
+DIR_SHARED_INITIATOR := initiator
 DIR_BASE := base
 DIR_BUSES := buses
 DIR_CONTROLLERS := controllers
@@ -215,9 +215,9 @@ vpath %.cpp $(VPATH) test
 vpath %.o $(OBJDIR)
 
 LIB_SHARED := $(LIBDIR)/libshared.a
-LIB_SHARED_PROTOBUF := $(LIBDIR)/libsharedprotobuf.a
-LIB_SHARED_COMMAND := $(LIBDIR)/libsharedcommand.a
-LIB_SHARED_INITIATOR := $(LIBDIR)/libsharedinitiator.a
+LIB_SHARED_PROTOBUF := $(LIBDIR)/libprotobuf.a
+LIB_SHARED_COMMAND := $(LIBDIR)/libcommand.a
+LIB_SHARED_INITIATOR := $(LIBDIR)/libinitiator.a
 LIB_BUS := $(LIBDIR)/libbus.a
 LIB_CONTROLLER := $(LIBDIR)/libcontroller.a
 LIB_DEVICE := $(LIBDIR)/libdevice.a
@@ -339,8 +339,8 @@ $(BINDIR)/$(S2P_TEST): $(LIB_SHARED_COMMAND) $(LIB_SHARED_INITIATOR) $(LIB_BUS)
 	$(CXX) $(CXXFLAGS) $(LDFLAGS) -o $@ $(OBJ_S2P_CORE) $(OBJ_S2PCTL_CORE) $(OBJ_S2P_TEST) $(LIB_SHARED_COMMAND) \
 		$(LIB_SHARED_INITIATOR) $(LIB_BUS) $(LIB_CONTROLLER) $(LIB_DEVICE) -lpthread -lpcap -lprotobuf -lgmock -lgtest
 
-$(BINDIR)/$(IN_PROCESS_TEST): $(LIB_SHARED_COMMAND) $(LIB_BUS) $(LIB_CONTROLLER) $(LIB_DEVICE) $(OBJ_S2P_CORE) \
-	$(OBJ_S2PCTL_CORE) $(OBJ_IN_PROCESS_TEST) | $(BINDIR)
+$(BINDIR)/$(IN_PROCESS_TEST): $(LIB_SHARED_COMMAND) $(LIB_SHARED_INITIATOR) $(LIB_BUS) $(LIB_CONTROLLER) $(LIB_DEVICE) \
+	$(OBJ_S2P_CORE) $(OBJ_S2PCTL_CORE) $(OBJ_IN_PROCESS_TEST) | $(BINDIR)
 	$(CXX) $(CXXFLAGS) $(LDFLAGS) -o $@ $(OBJ_S2P_CORE) $(OBJ_S2PCTL_CORE) $(OBJ_IN_PROCESS_TEST) $(LIB_SHARED_COMMAND) \
 		$(LIB_SHARED_INITIATOR) $(LIB_BUS) $(LIB_CONTROLLER) $(LIB_DEVICE) -lpthread -lpcap -lprotobuf
 

cpp/shared_command/command_dispatcher.cpp → cpp/command/command_dispatcher.cpp

@@ -10,7 +10,7 @@
 #include "controllers/controller_factory.h"
 #include "shared/s2p_util.h"
 #include "shared/shared_exceptions.h"
-#include "shared_protobuf/protobuf_util.h"
+#include "protobuf/protobuf_util.h"
 #include "command_dispatcher.h"
 
 using namespace std;

cpp/shared_command/command_dispatcher.h → cpp/command/command_dispatcher.h

@@ -8,7 +8,7 @@
 
 #pragma once
 
-#include "shared_protobuf/command_context.h"
+#include "protobuf/command_context.h"
 #include "command_executor.h"
 #include "image_support.h"
 #include "command_response.h"

cpp/shared_command/command_executor.cpp → cpp/command/command_executor.cpp

@@ -11,8 +11,8 @@
 #include "shared/s2p_util.h"
 #include "shared/localizer.h"
 #include "shared/shared_exceptions.h"
-#include "shared_protobuf/protobuf_util.h"
-#include "shared_protobuf/command_context.h"
+#include "protobuf/protobuf_util.h"
+#include "protobuf/command_context.h"
 #include "devices/disk.h"
 #include "command_executor.h"
 

cpp/shared_command/command_response.cpp → cpp/command/command_response.cpp

@@ -10,7 +10,7 @@
 #include <filesystem>
 #include "base/property_handler.h"
 #include "controllers/controller_factory.h"
-#include "shared_protobuf/protobuf_util.h"
+#include "protobuf/protobuf_util.h"
 #include "shared/network_util.h"
 #include "shared/s2p_util.h"
 #include "shared/s2p_version.h"

cpp/shared_command/image_support.cpp → cpp/command/image_support.cpp

@@ -11,7 +11,7 @@
 #include <array>
 #include "devices/disk.h"
 #include "shared/s2p_util.h"
-#include "shared_protobuf/protobuf_util.h"
+#include "protobuf/protobuf_util.h"
 #include "image_support.h"
 
 using namespace std;

cpp/shared_command/image_support.h → cpp/command/image_support.h

@@ -10,7 +10,7 @@
 
 #include <filesystem>
 // TODO Try to get rid of this dependency
-#include "shared_protobuf/command_context.h"
+#include "protobuf/command_context.h"
 
 using namespace std;
 using namespace filesystem;

cpp/devices/ctapdriver.cpp

@@ -86,7 +86,7 @@ bool CTapDriver::Init(const param_map &const_params)
     // IFF_NO_PI for no extra packet information
     ifreq ifr = { };
     ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
-    strncpy(ifr.ifr_name, DEFAULT_BRIDGE_IF.c_str(), IFNAMSIZ - 1); // NOSONAR Using strncpy is safe
+    strncpy(ifr.ifr_name, BRIDGE_INTERFACE_NAME.c_str(), IFNAMSIZ - 1); // NOSONAR Using strncpy is safe
 
     if (const int ret = ioctl(tap_fd, TUNSETIFF, (void*)&ifr); ret == -1) {
         LogErrno("Can't ioctl TUNSETIFF");
@@ -117,9 +117,9 @@ bool CTapDriver::Init(const param_map &const_params)
         return false;
     };
 
-    // Check if the bridge has already been created by checking whether there is a MAC address for the bridge.
+    // Check if the bridge has already been created by checking whether there is a MAC address for it
     if (GetMacAddress(BRIDGE_NAME).empty()) {
-        trace("Checking which interface is available for creating the bridge " + BRIDGE_NAME);
+        trace("Checking which interface is available for creating bridge " + BRIDGE_NAME);
 
         const auto &it = ranges::find_if(interfaces, [](const string &iface) {return IsInterfaceUp(iface);});
         if (it == interfaces.end()) {
@@ -143,59 +143,60 @@ bool CTapDriver::Init(const param_map &const_params)
         if (const string error = ip_link(ip_fd, BRIDGE_NAME.c_str(), true); !error.empty()) {
             return cleanUp(error);
         }
+
+        bridge_created = true;
     }
     else {
-        info(BRIDGE_NAME + " is already available");
+        info(BRIDGE_NAME + " already exists");
     }
 
-    trace(">ip link set " + DEFAULT_BRIDGE_IF + " up");
-    if (const string error = ip_link(ip_fd, DEFAULT_BRIDGE_IF.c_str(), true); !error.empty()) {
+    trace(">ip link set " + BRIDGE_INTERFACE_NAME + " up");
+    if (const string error = ip_link(ip_fd, BRIDGE_INTERFACE_NAME.c_str(), true); !error.empty()) {
         return cleanUp(error);
     }
 
-    trace(">brctl addif " + BRIDGE_NAME + " " + DEFAULT_BRIDGE_IF);
-    if (const string error = br_setif(br_socket_fd, BRIDGE_NAME, DEFAULT_BRIDGE_IF, true); !error.empty()) {
+    trace(">brctl addif " + BRIDGE_NAME + " " + BRIDGE_INTERFACE_NAME);
+    if (const string error = br_setif(br_socket_fd, BRIDGE_NAME, BRIDGE_INTERFACE_NAME, true); !error.empty()) {
         return cleanUp(error);
     }
 
     close(ip_fd);
     close(br_socket_fd);
 
-    info("Tap device " + DEFAULT_BRIDGE_IF + " created");
+    info("Tap device " + BRIDGE_INTERFACE_NAME + " created");
 
     return true;
 #endif
 }
 
 void CTapDriver::CleanUp() const
 {
-    if (tap_fd != -1) {
-        if (const int fd = socket(AF_LOCAL, SOCK_STREAM, 0); fd == -1) {
-            LogErrno("Can't open bridge socket");
-        } else {
-            trace(">brctl delif " + BRIDGE_NAME + " " + DEFAULT_BRIDGE_IF);
-            if (const string error = br_setif(fd, BRIDGE_NAME, DEFAULT_BRIDGE_IF, false); !error.empty()) {
-                warn("Removing " + DEFAULT_BRIDGE_IF + " from the bridge failed: " + error);
-                warn("You may need to manually remove the tap device");
-            }
+    if (tap_fd == -1) {
+        return;
+    }
 
-            trace(">ip link set dev " + BRIDGE_NAME + " down");
-            if (const string error = ip_link(fd, BRIDGE_NAME.c_str(), false); !error.empty()) {
-                warn(error);
-            }
+    if (const int fd = socket(AF_LOCAL, SOCK_STREAM, 0); fd == -1) {
+        LogErrno("Can't open bridge socket");
+    } else {
+        trace(">brctl delif " + BRIDGE_NAME + " " + BRIDGE_INTERFACE_NAME);
+        if (const string error = br_setif(fd, BRIDGE_NAME, BRIDGE_INTERFACE_NAME, false); !error.empty()) {
+            warn("Removing " + BRIDGE_INTERFACE_NAME + " from the bridge failed: " + error);
+            warn("You may need to manually remove the tap device");
+        }
 
-#ifdef __linux__
-            trace(">brctl delbr " + BRIDGE_NAME);
-            if (ioctl(fd, SIOCBRDELBR, BRIDGE_NAME.c_str()) == -1) {
-                warn("Removing " + BRIDGE_NAME + " failed: " + strerror(errno));
-            }
-#endif
+        trace(">ip link set dev " + BRIDGE_NAME + " down");
+        if (const string error = ip_link(fd, BRIDGE_NAME.c_str(), false); !error.empty()) {
+            warn(error);
+        }
 
-            close(fd);
+        if (const string error = DeleteBridge(fd); !error.empty()) {
+            warn(error);
         }
 
-        close(tap_fd);
+        close(fd);
     }
+
+    close(tap_fd);
 }
 
 param_map CTapDriver::GetDefaultParams() const
@@ -299,11 +300,25 @@ string CTapDriver::AddBridge(int fd)
     return "";
 }
 
+string CTapDriver::DeleteBridge(int fd) const
+{
+#ifdef __linux__
+    if (bridge_created) {
+        trace(">brctl delbr " + BRIDGE_NAME);
+        if (ioctl(fd, SIOCBRDELBR, BRIDGE_NAME.c_str()) == -1) {
+            return "Removing " + BRIDGE_NAME + " failed: " + strerror(errno);
+        }
+    }
+#endif
+
+    return "";
+}
+
 string CTapDriver::IpLink(bool enable)
 {
     const int fd = socket(PF_INET, SOCK_DGRAM, 0);
-    trace(string(">ip link set " + DEFAULT_BRIDGE_IF + " ") + (enable ? "up" : "down"));
-    const string result = ip_link(fd, DEFAULT_BRIDGE_IF.c_str(), enable);
+    trace(string(">ip link set " + BRIDGE_INTERFACE_NAME + " ") + (enable ? "up" : "down"));
+    const string result = ip_link(fd, BRIDGE_INTERFACE_NAME.c_str(), enable);
     close(fd);
 
     return result;

cpp/devices/ctapdriver.h

@@ -26,14 +26,12 @@ using namespace std;
 
 class CTapDriver
 {
+    const inline static string BRIDGE_INTERFACE_NAME = "piscsi0";
     const inline static string BRIDGE_NAME = "piscsi_bridge";
 
     const inline static string DEFAULT_IP = "10.10.20.1/24"; // NOSONAR This hardcoded IP address is safe
-
     const inline static string DEFAULT_NETMASK = "255.255.255.0"; // NOSONAR This hardcoded netmask is safe
 
-    const inline static string DEFAULT_BRIDGE_IF = "piscsi0";
-
 public:
 
     CTapDriver() = default;
@@ -60,8 +58,8 @@ class CTapDriver
         return BRIDGE_NAME;
     }
 
-    // Add the piscsi_bridge bridge
     static string AddBridge(int);
+    string DeleteBridge(int) const;
 
     // Enable/Disable the piscsi0 interface
     static string IpLink(bool);
@@ -79,5 +77,7 @@ class CTapDriver
     vector<string> interfaces;
 
     string inet;
+
+    bool bridge_created = false;
 };
 

cpp/devices/host_services.cpp

@@ -85,7 +85,7 @@
 #include <algorithm>
 #include <chrono>
 #include "shared/shared_exceptions.h"
-#include "shared_protobuf/protobuf_util.h"
+#include "protobuf/protobuf_util.h"
 #include "controllers/scsi_controller.h"
 #include "base/memory_util.h"
 #include "host_services.h"

cpp/devices/host_services.h

@@ -13,9 +13,9 @@
 #include <span>
 #include <vector>
 #include <map>
-#include "shared_protobuf/command_context.h"
-#include "shared_command/command_dispatcher.h"
-#include "shared_command/image_support.h"
+#include "protobuf/command_context.h"
+#include "command/command_dispatcher.h"
+#include "command/image_support.h"
 #include "mode_page_device.h"
 
 using namespace std;

cpp/shared_initiator/initiator_executor.cpp → cpp/initiator/initiator_executor.cpp

@@ -13,35 +13,29 @@
 using namespace std;
 using namespace spdlog;
 
-void InitiatorExecutor::Reset() const
+int InitiatorExecutor::Execute(scsi_command cmd, span<uint8_t> cdb, span<uint8_t> buffer, int length)
 {
-    bus.SetDAT(0);
-    bus.SetBSY(false);
-    bus.SetSEL(false);
-    bus.SetATN(false);
-}
+    bus.Reset();
 
-bool InitiatorExecutor::Execute(scsi_command cmd, span<uint8_t> cdb, span<uint8_t> buffer, int length, bool sasi)
-{
-    status = 0;
+    status = -1;
     byte_count = 0;
 
     if (const auto &command = COMMAND_MAPPING.find(cmd); command != COMMAND_MAPPING.end()) {
-        trace("Executing command {0} for target {1}:{2}", command->second.second, target_id, target_lun);
+        trace("Executing command {0} for device {1}:{2}", command->second.second, target_id, target_lun);
     }
     else {
-        trace("Executing command ${0:02x} for target {1}:{2}", static_cast<int>(cmd), target_id, target_lun);
+        trace("Executing command ${0:02x} for device {1}:{2}", static_cast<int>(cmd), target_id, target_lun);
     }
 
     // There is no arbitration phase with SASI
     if (!sasi && !Arbitration()) {
         bus.Reset();
-        return false;
+        return -1;
     }
 
-    if (!Selection(sasi)) {
-        Reset();
-        return false;
+    if (!Selection()) {
+        bus.Reset();
+        return -1;
     }
 
     // Timeout 3 s
@@ -55,19 +49,18 @@ bool InitiatorExecutor::Execute(scsi_command cmd, span<uint8_t> cdb, span<uint8_
                     now = chrono::steady_clock::now();
                 }
                 else {
-                    bus.Reset();
-                    return !status;
+                    return status;
                 }
             }
             catch (const phase_exception &e) {
                 error(e.what());
                 bus.Reset();
-                return false;
+                return -1;
             }
         }
     }
 
-    return false;
+    return -1;
 }
 
 bool InitiatorExecutor::Dispatch(scsi_command cmd, span<uint8_t> cdb, span<uint8_t> buffer, int length)
@@ -112,6 +105,8 @@ bool InitiatorExecutor::Dispatch(scsi_command cmd, span<uint8_t> cdb, span<uint8
 
 bool InitiatorExecutor::Arbitration() const
 {
+    trace("Arbitration with initiator ID {}", initiator_id);
+
     if (!WaitForFree()) {
         trace("Bus is not free");
         return false;
@@ -138,8 +133,10 @@ bool InitiatorExecutor::Arbitration() const
     return true;
 }
 
-bool InitiatorExecutor::Selection(bool sasi) const
+bool InitiatorExecutor::Selection() const
 {
+    trace("Selection of target {0} with initiator ID {1}", target_id, initiator_id);
+
     // There is no initiator ID with SASI
     bus.SetDAT(static_cast<uint8_t>((sasi ? 0 : 1 << initiator_id) + (1 << target_id)));
 
@@ -158,7 +155,7 @@ bool InitiatorExecutor::Selection(bool sasi) const
     }
 
     if (!WaitForBusy()) {
-        trace("SELECTION phase failed");
+        trace("Selection failed");
         return false;
     }