s2pdump: Support retries when dumping/restoring hard drives (#122)

cpp/base/property_handler.cpp

@@ -41,7 +41,7 @@ void PropertyHandler::Init(const string &filenames, const property_map &cmd_prop
     // Normalize properties by adding an explicit LUN where required
     for (const auto& [key, value] : properties) {
         const auto &components = Split(key, '.');
-        if (key.starts_with("device.") && key.find(":") == string::npos && components.size() == 3) {
+        if (key.starts_with(PropertyHandler::DEVICE) && key.find(":") == string::npos && components.size() == 3) {
             AddProperty(components[0] + "." + components[1] + ":0." + components[2], value);
         }
         else {

cpp/base/property_handler.h

@@ -38,6 +38,7 @@ class PropertyHandler
     static constexpr const char *TOKEN_FILE = "token_file";
 
     // Device-specific property keys
+    static constexpr const char *DEVICE = "device.";
     static constexpr const char *ACTIVE = "active";
     static constexpr const char *BLOCK_SIZE = "block_size";
     static constexpr const char *CACHING_MODE = "caching_mode";

cpp/command/command_dispatcher.cpp

@@ -89,8 +89,7 @@ bool CommandDispatcher::DispatchCommand(const CommandContext &context, PbResult
             return context.ReturnLocalizedError(LocalizationKey::ERROR_MISSING_FILENAME);
         }
         else {
-            if (const auto &image_file = make_unique<PbImageFile>(); GetImageFile(*image_file.get(),
-                filename)) {
+            if (const auto &image_file = make_unique<PbImageFile>(); GetImageFile(*image_file.get(), filename)) {
                 result.set_allocated_image_file_info(image_file.get());
                 result.set_status(true);
                 return context.WriteResult(result);

cpp/command/command_executor.cpp

@@ -156,14 +156,15 @@ bool CommandExecutor::Eject(PrimaryDevice &device) const
 {
     s2p_logger.info("Eject requested for {}", GetIdentifier(device));
 
-    if (!device.Eject(true)) {
-        s2p_logger.warn("Ejecting {} failed", GetIdentifier(device));
-        return true;
+    if (device.Eject(true)) {
+        // Remove both potential properties, with and without LUN
+        PropertyHandler::GetInstance().RemoveProperties(
+            fmt::format("{0}{1}:{2}.params", PropertyHandler::DEVICE, device.GetId(), device.GetLun()));
+        PropertyHandler::GetInstance().RemoveProperties(
+            fmt::format("{0}{1}.params", PropertyHandler::DEVICE, device.GetId()));
     }
-
-    PropertyHandler::GetInstance().RemoveProperties(fmt::format("device.{0}:{1}.params", device.GetId(), device.GetLun()));
-    if (!device.GetLun()) {
-        PropertyHandler::GetInstance().RemoveProperties(fmt::format("device.{}.params", device.GetId()));
+    else {
+        s2p_logger.warn("Ejecting {} failed", GetIdentifier(device));
     }
 
     return true;
@@ -205,16 +206,14 @@ bool CommandExecutor::Attach(const CommandContext &context, const PbDeviceDefini
         return context.ReturnLocalizedError(LocalizationKey::ERROR_RESERVED_ID, to_string(id));
     }
 
-    const string &filename = GetParam(pb_device, "file");
-
-    const auto device = CreateDevice(context, pb_device, filename);
+    const auto device = CreateDevice(context, pb_device);
     if (!device) {
         return false;
     }
 
     param_map params = { pb_device.params().cbegin(), pb_device.params().cend() };
     if (!device->SupportsImageFile()) {
-        // Legacy clients like scsictl might have sent both "file" and "interfaces"
+        // Legacy clients like PiSCSI's scsictl might have sent both "file" and "interfaces"
         params.erase("file");
     }
     device->SetParams(params);
@@ -226,7 +225,7 @@ bool CommandExecutor::Attach(const CommandContext &context, const PbDeviceDefini
         return false;
     }
 
-    if (!SetScsiLevel(context, device, pb_device.scsi_level())) {
+    if (!SetScsiLevel(context, *device, pb_device.scsi_level())) {
         return false;
     }
 
@@ -240,6 +239,8 @@ bool CommandExecutor::Attach(const CommandContext &context, const PbDeviceDefini
 
 #ifdef BUILD_STORAGE_DEVICE
     if (device->SupportsImageFile()) {
+        const string &filename = GetParam(pb_device, "file");
+
         // If no filename was provided the medium is considered not inserted
         device->SetRemoved(filename.empty());
 
@@ -279,7 +280,7 @@ bool CommandExecutor::Attach(const CommandContext &context, const PbDeviceDefini
     }
 
     // Set the final data (they may have been overriden during the initialization of SCSG)
-    SetScsiLevel(context, device, pb_device.scsi_level());
+    SetScsiLevel(context, *device, pb_device.scsi_level());
     SetProductData(context, pb_device, *device);
 
     if (!controller_factory.AttachToController(bus, id, device)) {
@@ -302,7 +303,7 @@ bool CommandExecutor::Attach(const CommandContext &context, const PbDeviceDefini
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-parameter"
 bool CommandExecutor::Insert(const CommandContext &context, const PbDeviceDefinition &pb_device,
-    const shared_ptr<PrimaryDevice> &device, bool dryRun) const
+    const shared_ptr<PrimaryDevice> device, bool dryRun) const
 {
     if (!device->SupportsImageFile()) {
         return false;
@@ -363,17 +364,15 @@ bool CommandExecutor::Insert(const CommandContext &context, const PbDeviceDefini
 bool CommandExecutor::Detach(const CommandContext &context, PrimaryDevice &device, bool dryRun) const
 {
     auto *controller = device.GetController();
-    if (!controller) {
-        return context.ReturnLocalizedError(LocalizationKey::ERROR_DETACH);
-    }
+    assert(controller);
 
     // LUN 0 can only be detached if there is no other LUN anymore
     if (!device.GetLun() && controller->GetLunCount() > 1) {
         return context.ReturnLocalizedError(LocalizationKey::ERROR_LUN0);
     }
 
     if (!dryRun) {
-        // Remember some device data before they become invalid on removal
+        // Remember device data before they become invalid on removal
         const int id = device.GetId();
         const int lun = device.GetLun();
         const string &identifier = GetIdentifier(device) + ", " + device.GetIdentifier();
@@ -382,17 +381,15 @@ bool CommandExecutor::Detach(const CommandContext &context, PrimaryDevice &devic
             return context.ReturnLocalizedError(LocalizationKey::ERROR_DETACH);
         }
 
+        // Remove both potential identifiers
+        PropertyHandler::GetInstance().RemoveProperties(fmt::format("{0}{1}:{2}.", PropertyHandler::DEVICE, id, lun));
+        PropertyHandler::GetInstance().RemoveProperties(fmt::format("{0}{1}.", PropertyHandler::DEVICE, id));
+
         // If no LUN is left also delete the controller
         if (!controller->GetLunCount() && !controller_factory.DeleteController(*controller)) {
             return context.ReturnLocalizedError(LocalizationKey::ERROR_DETACH);
         }
 
-        // Consider both potential identifiers if the LUN is 0
-        PropertyHandler::GetInstance().RemoveProperties(fmt::format("device.{0}:{1}.", id, lun));
-        if (!lun) {
-            PropertyHandler::GetInstance().RemoveProperties(fmt::format("device.{}.", id));
-        }
-
         s2p_logger.info("Detached " + identifier);
     }
 
@@ -402,15 +399,15 @@ bool CommandExecutor::Detach(const CommandContext &context, PrimaryDevice &devic
 void CommandExecutor::DetachAll() const
 {
     if (controller_factory.DeleteAllControllers()) {
-        PropertyHandler::GetInstance().RemoveProperties("device.");
+        PropertyHandler::GetInstance().RemoveProperties(PropertyHandler::DEVICE);
 
         s2p_logger.info("Detached all devices");
     }
 }
 
 void CommandExecutor::SetUpDeviceProperties(shared_ptr<PrimaryDevice> device)
 {
-    const string &identifier = fmt::format("device.{0}:{1}.", device->GetId(), device->GetLun());
+    const string &identifier = fmt::format("{0}{1}:{2}.", PropertyHandler::DEVICE, device->GetId(), device->GetLun());
     PropertyHandler::GetInstance().AddProperty(identifier + "type", GetTypeString(*device));
     const auto& [product, vendor, revision] = device->GetProductData();
     PropertyHandler::GetInstance().AddProperty(identifier + "name", vendor + ":" + product + ":" + revision);
@@ -622,8 +619,10 @@ bool CommandExecutor::EnsureLun0(const CommandContext &context, const PbCommand
 }
 
 shared_ptr<PrimaryDevice> CommandExecutor::CreateDevice(const CommandContext &context,
-    const PbDeviceDefinition &pb_device, const string &filename) const
+    const PbDeviceDefinition &pb_device) const
 {
+    const string &filename = GetParam(pb_device, "file");
+
     auto device = DeviceFactory::GetInstance().CreateDevice(pb_device.type(), pb_device.unit(), filename);
     if (!device) {
         if (pb_device.type() == UNDEFINED) {
@@ -651,9 +650,9 @@ shared_ptr<PrimaryDevice> CommandExecutor::CreateDevice(const CommandContext &co
     return device;
 }
 
-bool CommandExecutor::SetScsiLevel(const CommandContext &context, shared_ptr<PrimaryDevice> device, int level) const
+bool CommandExecutor::SetScsiLevel(const CommandContext &context, PrimaryDevice &device, int level) const
 {
-    if (level && !device->SetScsiLevel(static_cast<ScsiLevel>(level))) {
+    if (level && !device.SetScsiLevel(static_cast<ScsiLevel>(level))) {
         return context.ReturnLocalizedError(LocalizationKey::ERROR_SCSI_LEVEL, to_string(level));
     }
 

cpp/command/command_executor.h

@@ -57,16 +57,16 @@ class CommandExecutor
     bool Protect(PrimaryDevice&) const;
     bool Unprotect(PrimaryDevice&) const;
     bool Attach(const CommandContext&, const PbDeviceDefinition&, bool);
-    bool Insert(const CommandContext&, const PbDeviceDefinition&, const shared_ptr<PrimaryDevice>&, bool) const;
+    bool Insert(const CommandContext&, const PbDeviceDefinition&, const shared_ptr<PrimaryDevice>, bool) const;
     bool Detach(const CommandContext&, PrimaryDevice&, bool) const;
     void DetachAll() const;
     string SetReservedIds(const string&);
-    #ifdef BUILD_STORAGE_DEVICE
+#ifdef BUILD_STORAGE_DEVICE
     bool ValidateImageFile(const CommandContext&, StorageDevice&, const string&) const;
 #endif
     bool EnsureLun0(const CommandContext&, const PbCommand&) const;
     bool ValidateDevice(const CommandContext&, const PbDeviceDefinition&) const;
-    shared_ptr<PrimaryDevice> CreateDevice(const CommandContext&, const PbDeviceDefinition&, const string&) const;
+    shared_ptr<PrimaryDevice> CreateDevice(const CommandContext&, const PbDeviceDefinition&) const;
     bool SetBlockSize(const CommandContext&, shared_ptr<PrimaryDevice>, int) const;
 
     mutex& GetExecutionLocker()
@@ -80,7 +80,7 @@ class CommandExecutor
 
 protected:
 
-    bool SetScsiLevel(const CommandContext&, shared_ptr<PrimaryDevice>, int) const;
+    bool SetScsiLevel(const CommandContext&, PrimaryDevice&, int) const;
 
 private:
 

cpp/controllers/controller.cpp

@@ -52,15 +52,7 @@ bool Controller::Process()
         return false;
     }
 
-    if (IsBusFree()) {
-        if (script_generator) {
-            script_generator->WriteEol();
-        }
-
-        return false;
-    }
-
-    return true;
+    return !IsBusFree();
 }
 
 void Controller::BusFree()

cpp/controllers/script_generator.cpp

@@ -25,10 +25,10 @@ void ScriptGenerator::AddCdb(int id, int lun, cdb_t cdb)
 {
     assert(!cdb.empty());
 
-    file << dec << "-i " << id << COMPONENT_SEPARATOR << lun << " -c " << hex;
+    file << "\n-i " << dec << id << COMPONENT_SEPARATOR << lun << " -c " << hex;
 
     int count = CommandMetaData::GetInstance().GetByteCount(static_cast<ScsiCommand>(cdb[0]));
-    // In case of an unknown command add all available CDB data
+    // In case of an unknown command add the complete CDB
     if (!count) {
         count = static_cast<int>(cdb.size());
     }
@@ -63,8 +63,3 @@ void ScriptGenerator::AddData(span<const uint8_t> data)
 
     file << flush;
 }
-
-void ScriptGenerator::WriteEol()
-{
-    file << '\n' << flush;
-}

cpp/controllers/script_generator.h

@@ -2,7 +2,7 @@
 //
 // SCSI2Pi, SCSI device emulator and SCSI tools for the Raspberry Pi
 //
-// Copyright (C) 2024 Uwe Seimet
+// Copyright (C) 2024-2025 Uwe Seimet
 //
 //---------------------------------------------------------------------------
 
@@ -23,8 +23,6 @@ class ScriptGenerator
     void AddCdb(int, int, cdb_t);
     void AddData(span<const uint8_t>);
 
-    void WriteEol();
-
 private:
 
     ofstream file;

cpp/s2p/s2p_core.cpp

@@ -192,7 +192,7 @@ int S2p::Run(span<char*> args, bool in_process, bool log_signals)
     }
 
     for (const auto& [key, value] : property_handler.GetUnknownProperties()) {
-        if (!key.starts_with("device.")) {
+        if (!key.starts_with(PropertyHandler::DEVICE)) {
             cerr << "Error: Invalid global property \"" << key << "\", check your command line and "
                 << PropertyHandler::CONFIGURATION << '\n';
             CleanUp();
@@ -263,7 +263,7 @@ bool S2p::ParseProperties(const property_map &properties, int &port, bool ignore
         }
 
         if (const string &scan_depth = property_handler.RemoveProperty(PropertyHandler::SCAN_DEPTH, "1"); !scan_depth.empty()) {
-            if (const int depth = ParseAsUnsignedInt(scan_depth); depth == -1) {
+            if (const int depth = ParseAsUnsignedInt(scan_depth); depth < 0) {
                 throw ParserException("Invalid image file scan depth: " + scan_depth);
             }
             else {
@@ -273,9 +273,9 @@ bool S2p::ParseProperties(const property_map &properties, int &port, bool ignore
 
         if (const string &script_file = property_handler.RemoveProperty(PropertyHandler::SCRIPT_FILE); !script_file.empty()) {
             if (!controller_factory.SetScriptFile(script_file)) {
-                throw ParserException("Can't create s2pexec script file '" + script_file + "': " + strerror(errno));
+                throw ParserException("Can't create script file '" + script_file + "': " + strerror(errno));
             }
-            s2p_logger->info("Generating s2pexec script file '" + script_file + "'");
+            s2p_logger->info("Generating script file '" + script_file + "'");
         }
 
         const string &p = property_handler.RemoveProperty(PropertyHandler::PORT, "6868");
@@ -349,11 +349,8 @@ void S2p::CreateDevices()
     int id = -1;
     int lun = -1;
     bool is_active = false;
-    for (const property_map &properties = property_handler.GetProperties(); const auto& [key, value] : properties) {
-        if (!key.starts_with("device.")) {
-            continue;
-        }
-
+    for (const property_map &properties = property_handler.GetProperties(PropertyHandler::DEVICE);
+        const auto& [key, value] : properties) {
         const auto &key_components = Split(key, '.', 3);
         if (key_components.size() < 3) {
             throw ParserException(fmt::format("Invalid device definition '{}'", key));
@@ -414,7 +411,7 @@ void S2p::AttachInitialDevices(PbCommand &command)
 
 bool S2p::CheckActive(const property_map &properties, const string &id_and_lun)
 {
-    if (const auto &it = properties.find("device." + id_and_lun + ".active"); it != properties.end()) {
+    if (const auto &it = properties.find(PropertyHandler::DEVICE + id_and_lun + ".active"); it != properties.end()) {
         const string &active = it->second;
         if (active != "true" && active != "false") {
             throw ParserException(fmt::format("Invalid boolean: '{}'", active));
@@ -434,16 +431,15 @@ void S2p::SetDeviceProperties(PbDeviceDefinition &device, const string &key, con
         device.set_type(ParseDeviceType(value));
     }
     else if (key == PropertyHandler::SCSI_LEVEL) {
-        if (const int level = ParseAsUnsignedInt(value); level == -1 || !level
-            || level >= static_cast<int>(ScsiLevel::LAST)) {
+        if (const int level = ParseAsUnsignedInt(value); level <= 0 || level >= static_cast<int>(ScsiLevel::LAST)) {
             throw ParserException(fmt::format("Invalid SCSI level: '{}'", value));
         }
         else {
             device.set_scsi_level(level);
         }
     }
     else if (key == PropertyHandler::BLOCK_SIZE) {
-        if (const int block_size = ParseAsUnsignedInt(value); block_size == -1) {
+        if (const int block_size = ParseAsUnsignedInt(value); block_size < 0) {
             throw ParserException(fmt::format("Invalid block size: '{}'", value));
         }
         else {

cpp/s2p/s2p_parser.cpp

@@ -73,7 +73,7 @@ string ParseBlueScsiFilename(property_map &properties, const string &d, const st
             lun = ParseNumber(type_id_lun.substr(3));
         }
         lun = !lun.empty() && lun != "0" ? ":" + lun : "";
-        device_key = fmt::format("device.{0}{1}.", id, lun);
+        device_key = fmt::format("{0}{1}{2}.", PropertyHandler::DEVICE, id, lun);
     }
 
     const string &type = type_id_lun.substr(0, 2);
@@ -307,7 +307,7 @@ property_map s2p_parser::ParseArguments(span<char*> initial_args, bool &ignore_c
             break;
         }
 
-        string device_key = id_lun.empty() ? "" : fmt::format("device.{}.", id_lun);
+        string device_key = id_lun.empty() ? "" : fmt::format("{0}{1}.", PropertyHandler::DEVICE, id_lun);
         const string &params = optarg;
         if (blue_scsi_mode && !params.empty()) {
             device_key = ParseBlueScsiFilename(properties, device_key, params);

cpp/s2pctl/sp2ctl_core.cpp

@@ -418,7 +418,7 @@ int S2pCtl::ParseArguments(const vector<char*> &args) // NOSONAR Acceptable comp
             break;
 
         case OPT_SCSI_LEVEL:
-            if (const int level = ParseAsUnsignedInt(optarg); level == -1 || !level
+            if (const int level = ParseAsUnsignedInt(optarg); level <= 0
                 || level >= static_cast<int>(ScsiLevel::LAST)) {
                 cerr << "Error: Invalid SCSI level '" << optarg << "'\n";
                 return EXIT_FAILURE;