Improve how commands are internally executed (#1247)

* Improve how commands are internally executed * Use const CommandContext for execution * Update error handling * Fix SonarQube issues * Remove duplicate code * Use mutex instead of atomic_bool * Add hasher * Add param_map * Do not log unknown operations as an error for backward/foward compatibility
PiSCSI · Oct 30, 2023 · 8bd06ea · 8bd06ea
1 parent b7cb23e
commit 8bd06ea
Show file tree

Hide file tree

Showing 13 changed files with 163 additions and 260 deletions.
diff --git a/cpp/controllers/scsi_controller.cpp b/cpp/controllers/scsi_controller.cpp
@@ -411,8 +411,8 @@ void ScsiController::Error(sense_key sense_key, asc asc, status status)
 
 	if (sense_key != sense_key::no_sense || asc != asc::no_additional_sense_information) {
 		stringstream s;
-		s << setfill('0') << setw(2) << hex << "Error status: Sense Key $" << static_cast<int>(sense_key)
-				<< ", ASC $" << static_cast<int>(asc);
+		s << setfill('0') << hex << "Error status: Sense Key $" << setw(2) << static_cast<int>(sense_key)
+				<< ", ASC $" << setw(2) << static_cast<int>(asc);
 		LogDebug(s.str());
 
 		// Set Sense Key and ASC for a subsequent REQUEST SENSE
@@ -422,8 +422,6 @@ void ScsiController::Error(sense_key sense_key, asc asc, status status)
 	SetStatus(status);
 	SetMessage(0x00);
 
-	LogTrace("Error (to status phase)");
-
 	Status();
 }
 
@@ -478,24 +476,19 @@ void ScsiController::Send()
 		case phase_t::msgin:
 			// Completed sending response to extended message of IDENTIFY message
 			if (scsi.atnmsg) {
-				// flag off
 				scsi.atnmsg = false;
 
-				// command phase
 				Command();
 			} else {
-				// Bus free phase
 				BusFree();
 			}
 			break;
 
 		case phase_t::datain:
-			// status phase
 			Status();
 			break;
 
 		case phase_t::status:
-			// Message in phase
 			SetLength(1);
 			SetBlocks(1);
 			GetBuffer()[0] = (uint8_t)GetMessage();

diff --git a/cpp/piscsi/command_context.cpp b/cpp/piscsi/command_context.cpp
@@ -43,10 +43,11 @@ void CommandContext::WriteResult(const PbResult& result) const
 	}
 }
 
-void CommandContext::WriteSuccessResult(PbResult& result) const
+bool CommandContext::WriteSuccessResult(PbResult& result) const
 {
 	result.set_status(true);
 	WriteResult(result);
+	return true;
 }
 
 bool CommandContext::ReturnLocalizedError(LocalizationKey key, const string& arg1, const string& arg2,
@@ -59,7 +60,13 @@ bool CommandContext::ReturnLocalizedError(LocalizationKey key, PbErrorCode error
 		const string& arg2, const string& arg3) const
 {
 	// For the logfile always use English
-	spdlog::error(localizer.Localize(key, "en", arg1, arg2, arg3));
+	// Do not log unknown operations as an error for backward/foward compatibility with old/new clients
+	if (error_code == PbErrorCode::UNKNOWN_OPERATION) {
+		spdlog::trace(localizer.Localize(key, "en", arg1, arg2, arg3));
+	}
+	else {
+		spdlog::error(localizer.Localize(key, "en", arg1, arg2, arg3));
+	}
 
 	return ReturnStatus(false, localizer.Localize(key, locale, arg1, arg2, arg3), error_code, false);
 }

diff --git a/cpp/piscsi/command_context.h b/cpp/piscsi/command_context.h
@@ -29,7 +29,7 @@ class CommandContext
 	void SetDefaultFolder(string_view f) { default_folder = f; }
 	bool ReadCommand();
 	void WriteResult(const PbResult&) const;
-	void WriteSuccessResult(PbResult&) const;
+	bool WriteSuccessResult(PbResult&) const;
 	const PbCommand& GetCommand() const { return command; }
 
 	bool ReturnLocalizedError(LocalizationKey, const string& = "", const string& = "", const string& = "") const;

diff --git a/cpp/piscsi/localizer.cpp b/cpp/piscsi/localizer.cpp
@@ -22,12 +22,12 @@ Localizer::Localizer()
 	Add(LocalizationKey::ERROR_AUTHENTICATION, "es", "Fallo de autentificación");
 	Add(LocalizationKey::ERROR_AUTHENTICATION, "zh", "认证失败");
 
-	Add(LocalizationKey::ERROR_OPERATION, "en", "Unknown operation");
-	Add(LocalizationKey::ERROR_OPERATION, "de", "Unbekannte Operation");
-	Add(LocalizationKey::ERROR_OPERATION, "sv", "Okänd operation");
-	Add(LocalizationKey::ERROR_OPERATION, "fr", "Opération inconnue");
-	Add(LocalizationKey::ERROR_OPERATION, "es", "Operación desconocida");
-	Add(LocalizationKey::ERROR_OPERATION, "zh", "未知操作");
+	Add(LocalizationKey::ERROR_OPERATION, "en", "Unknown operation: %1");
+	Add(LocalizationKey::ERROR_OPERATION, "de", "Unbekannte Operation: %1");
+	Add(LocalizationKey::ERROR_OPERATION, "sv", "Okänd operation: %1");
+	Add(LocalizationKey::ERROR_OPERATION, "fr", "Opération inconnue: %1");
+	Add(LocalizationKey::ERROR_OPERATION, "es", "Operación desconocida: %1");
+	Add(LocalizationKey::ERROR_OPERATION, "zh", "未知操作: %1");
 
 	Add(LocalizationKey::ERROR_LOG_LEVEL, "en", "Invalid log level '%1'");
 	Add(LocalizationKey::ERROR_LOG_LEVEL, "de", "Ungültiger Log-Level '%1'");