From 8bd06ea5cd9326349d2271d1a440bd1c7605f057 Mon Sep 17 00:00:00 2001 From: Uwe Seimet <48174652+uweseimet@users.noreply.github.com> Date: Mon, 30 Oct 2023 13:57:46 +0100 Subject: [PATCH] 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 --- cpp/controllers/scsi_controller.cpp | 11 +- cpp/piscsi/command_context.cpp | 11 +- cpp/piscsi/command_context.h | 2 +- cpp/piscsi/localizer.cpp | 12 +-- cpp/piscsi/piscsi_core.cpp | 157 +++++++++++++++++++--------- cpp/piscsi/piscsi_core.h | 13 +-- cpp/piscsi/piscsi_executor.cpp | 96 ++--------------- cpp/piscsi/piscsi_executor.h | 14 +-- cpp/piscsi/piscsi_image.cpp | 4 + cpp/test/mocks.h | 5 - cpp/test/piscsi_executor_test.cpp | 96 +++-------------- cpp/test/piscsi_response_test.cpp | 1 + cpp/test/test_shared.cpp | 1 - 13 files changed, 163 insertions(+), 260 deletions(-) diff --git a/cpp/controllers/scsi_controller.cpp b/cpp/controllers/scsi_controller.cpp index e6d1f45c..26bb553e 100644 --- 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(sense_key) - << ", ASC $" << static_cast(asc); + s << setfill('0') << hex << "Error status: Sense Key $" << setw(2) << static_cast(sense_key) + << ", ASC $" << setw(2) << static_cast(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 index 5b381d9f..ab4eeac4 100644 --- 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 index dcbb2d20..25c4dbb8 100644 --- a/cpp/piscsi/command_context.h +++ b/cpp/piscsi/command_context.h @@ -29,7 +29,7 @@ public: 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 index 17c92034..1ac8f5f7 100644 --- 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'"); diff --git a/cpp/piscsi/piscsi_core.cpp b/cpp/piscsi/piscsi_core.cpp index 1be72793..553e2297 100644 --- a/cpp/piscsi/piscsi_core.cpp +++ b/cpp/piscsi/piscsi_core.cpp @@ -33,6 +33,7 @@ using namespace std; using namespace filesystem; +using namespace spdlog; using namespace piscsi_interface; using namespace piscsi_util; using namespace protobuf_util; @@ -71,7 +72,7 @@ bool Piscsi::InitBus() return false; } - executor = make_unique(piscsi_image, *bus, controller_manager); + executor = make_unique(*bus, controller_manager); return true; } @@ -84,7 +85,12 @@ void Piscsi::CleanUp() executor->DetachAll(); - bus->Cleanup(); + // TODO Check why there are rare cases where bus is NULL on a remote interface shutdown + // even though it is never set to NULL anywhere + assert(bus); + if (bus) { + bus->Cleanup(); + } } void Piscsi::ReadAccessToken(const path& filename) @@ -307,27 +313,26 @@ bool Piscsi::SetLogLevel(const string& log_level) const return true; } -bool Piscsi::ExecuteCommand(CommandContext& context) +bool Piscsi::ExecuteCommand(const CommandContext& context) { - context.SetDefaultFolder(piscsi_image.GetDefaultFolder()); - const PbCommand& command = context.GetCommand(); + const PbOperation operation = command.operation(); if (!access_token.empty() && access_token != GetParam(command, "token")) { return context.ReturnLocalizedError(LocalizationKey::ERROR_AUTHENTICATION, UNAUTHORIZED); } - if (!PbOperation_IsValid(command.operation())) { - spdlog::error("Received unknown command with operation opcode " + to_string(command.operation())); + if (!PbOperation_IsValid(operation)) { + spdlog::trace("Ignored unknown command with operation opcode " + to_string(operation)); - return context.ReturnLocalizedError(LocalizationKey::ERROR_OPERATION, UNKNOWN_OPERATION); + return context.ReturnLocalizedError(LocalizationKey::ERROR_OPERATION, UNKNOWN_OPERATION, to_string(operation)); } - spdlog::trace("Received " + PbOperation_Name(command.operation()) + " command"); + spdlog::trace("Received " + PbOperation_Name(operation) + " command"); PbResult result; - switch(command.operation()) { + switch(operation) { case LOG_LEVEL: if (const string log_level = GetParam(command, "level"); !SetLogLevel(log_level)) { context.ReturnLocalizedError(LocalizationKey::ERROR_LOG_LEVEL, log_level); @@ -353,8 +358,7 @@ bool Piscsi::ExecuteCommand(CommandContext& context) case DEVICE_TYPES_INFO: response.GetDeviceTypesInfo(*result.mutable_device_types_info()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case SERVER_INFO: response.GetServerInfo(*result.mutable_server_info(), command, controller_manager.GetAllDevices(), @@ -364,19 +368,16 @@ bool Piscsi::ExecuteCommand(CommandContext& context) case VERSION_INFO: response.GetVersionInfo(*result.mutable_version_info()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case LOG_LEVEL_INFO: response.GetLogLevelInfo(*result.mutable_log_level_info()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case DEFAULT_IMAGE_FILES_INFO: response.GetImageFilesInfo(*result.mutable_image_files_info(), piscsi_image.GetDefaultFolder(), GetParam(command, "folder_pattern"), GetParam(command, "file_pattern"), piscsi_image.GetDepth()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case IMAGE_FILE_INFO: if (string filename = GetParam(command, "file"); filename.empty()) { @@ -399,13 +400,11 @@ bool Piscsi::ExecuteCommand(CommandContext& context) case NETWORK_INTERFACES_INFO: response.GetNetworkInterfacesInfo(*result.mutable_network_interfaces_info()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case MAPPING_INFO: response.GetMappingInfo(*result.mutable_mapping_info()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case STATISTICS_INFO: response.GetStatisticsInfo(*result.mutable_statistics_info(), controller_manager.GetAllDevices()); @@ -414,45 +413,65 @@ bool Piscsi::ExecuteCommand(CommandContext& context) case OPERATION_INFO: response.GetOperationInfo(*result.mutable_operation_info(), piscsi_image.GetDepth()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case RESERVED_IDS_INFO: response.GetReservedIds(*result.mutable_reserved_ids_info(), executor->GetReservedIds()); - context.WriteSuccessResult(result); - break; + return context.WriteSuccessResult(result); case SHUT_DOWN: - if (executor->ShutDown(context, GetParam(command, "mode"))) { - TerminationHandler(0); - } - break; + return ShutDown(context, GetParam(command, "mode")); case NO_OPERATION: - context.ReturnSuccessStatus(); - break; + return context.ReturnSuccessStatus(); - // TODO The image operations below can most likely directly be executed without calling the executor, - // because they do not require the target to be idle case CREATE_IMAGE: + return piscsi_image.CreateImage(context); + case DELETE_IMAGE: + return piscsi_image.DeleteImage(context); + case RENAME_IMAGE: + return piscsi_image.RenameImage(context); + case COPY_IMAGE: + return piscsi_image.CopyImage(context); + case PROTECT_IMAGE: case UNPROTECT_IMAGE: + return piscsi_image.SetImagePermissions(context); + case RESERVE_IDS: return executor->ProcessCmd(context); - // The remaining commands can only be executed when the target is idle - // TODO What happens when the target becomes active while the command is still being executed? - // A field 'mutex locker' can probably avoid SCSI commands and ProcessCmd() being executed at the same time default: - // TODO Find a better way to wait - const timespec ts = { .tv_sec = 0, .tv_nsec = 500'000'000}; - while (target_is_active) { - nanosleep(&ts, nullptr); + // The remaining commands may only be executed when the target is idle + if (!ExecuteWithLock(context)) { + return false; } - return executor->ProcessCmd(context); + + return HandleDeviceListChange(context, operation); + } + + return true; +} + +bool Piscsi::ExecuteWithLock(const CommandContext& context) +{ + scoped_lock lock(execution_locker); + return executor->ProcessCmd(context); +} + +bool Piscsi::HandleDeviceListChange(const CommandContext& context, PbOperation operation) const +{ + // ATTACH and DETACH return the resulting device list + if (operation == ATTACH || operation == DETACH) { + // A command with an empty device list is required here in order to return data for all devices + PbCommand command; + PbResult result; + response.GetDevicesInfo(controller_manager.GetAllDevices(), result, command, piscsi_image.GetDefaultFolder()); + context.WriteResult(result); + return result.status(); } return true; @@ -489,8 +508,10 @@ int Piscsi::run(span args) return EXIT_FAILURE; } - if (const string error = service.Init([this] (CommandContext& context) { return ExecuteCommand(context); }, port); - !error.empty()) { + if (const string error = service.Init([this] (CommandContext& context) { + context.SetDefaultFolder(piscsi_image.GetDefaultFolder()); + return ExecuteCommand(context); + }, port); !error.empty()) { cerr << "Error: " << error << endl; CleanUp(); @@ -584,7 +605,7 @@ void Piscsi::Process() // Only process the SCSI command if the bus is not busy and no other device responded if (IsNotBusy() && bus->GetSEL()) { - target_is_active = true; + scoped_lock lock(execution_locker); // Process command on the responsible controller based on the current initiator and target ID if (const auto shutdown_mode = controller_manager.ProcessOnController(bus->GetDAT()); @@ -592,23 +613,54 @@ void Piscsi::Process() // When the bus is free PiSCSI or the Pi may be shut down. ShutDown(shutdown_mode); } - - target_is_active = false; } } } -void Piscsi::ShutDown(AbstractController::piscsi_shutdown_mode shutdown_mode) -{ - CleanUp(); +// Shutdown on a remote interface command +bool Piscsi::ShutDown(const CommandContext& context, const string& m) { + if (m.empty()) { + return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_MODE_MISSING); + } + AbstractController::piscsi_shutdown_mode mode = AbstractController::piscsi_shutdown_mode::NONE; + if (m == "rascsi") { + mode = AbstractController::piscsi_shutdown_mode::STOP_PISCSI; + } + else if (m == "system") { + mode = AbstractController::piscsi_shutdown_mode::STOP_PI; + } + else if (m == "reboot") { + mode = AbstractController::piscsi_shutdown_mode::RESTART_PI; + } + else { + return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_MODE_INVALID, m); + } + + // Shutdown modes other than rascsi require root permissions + if (mode != AbstractController::piscsi_shutdown_mode::STOP_PISCSI && getuid()) { + return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_PERMISSION); + } + + // Report success now because after a shutdown nothing can be reported anymore + PbResult result; + context.WriteSuccessResult(result); + + return ShutDown(mode); +} + +// Shutdown on a SCSI command +bool Piscsi::ShutDown(AbstractController::piscsi_shutdown_mode shutdown_mode) +{ switch(shutdown_mode) { case AbstractController::piscsi_shutdown_mode::STOP_PISCSI: spdlog::info("PiSCSI shutdown requested"); - break; + CleanUp(); + return true; case AbstractController::piscsi_shutdown_mode::STOP_PI: spdlog::info("Raspberry Pi shutdown requested"); + CleanUp(); if (system("init 0") == -1) { spdlog::error("Raspberry Pi shutdown failed"); } @@ -616,6 +668,7 @@ void Piscsi::ShutDown(AbstractController::piscsi_shutdown_mode shutdown_mode) case AbstractController::piscsi_shutdown_mode::RESTART_PI: spdlog::info("Raspberry Pi restart requested"); + CleanUp(); if (system("init 6") == -1) { spdlog::error("Raspberry Pi restart failed"); } @@ -625,6 +678,8 @@ void Piscsi::ShutDown(AbstractController::piscsi_shutdown_mode shutdown_mode) assert(false); break; } + + return false; } bool Piscsi::IsNotBusy() const diff --git a/cpp/piscsi/piscsi_core.h b/cpp/piscsi/piscsi_core.h index fea8dd73..1560851d 100644 --- a/cpp/piscsi/piscsi_core.h +++ b/cpp/piscsi/piscsi_core.h @@ -10,7 +10,6 @@ #pragma once #include "controllers/controller_manager.h" -#include "controllers/abstract_controller.h" #include "piscsi/command_context.h" #include "piscsi/piscsi_service.h" #include "piscsi/piscsi_image.h" @@ -20,7 +19,7 @@ #include "spdlog/sinks/stdout_color_sinks.h" #include #include -#include +#include using namespace std; @@ -49,9 +48,12 @@ private: void Process(); bool IsNotBusy() const; - void ShutDown(AbstractController::piscsi_shutdown_mode); + bool ShutDown(AbstractController::piscsi_shutdown_mode); + bool ShutDown(const CommandContext&, const string&); - bool ExecuteCommand(CommandContext&); + bool ExecuteCommand(const CommandContext&); + bool ExecuteWithLock(const CommandContext&); + bool HandleDeviceListChange(const CommandContext&, PbOperation) const; bool SetLogLevel(const string&) const; @@ -59,8 +61,7 @@ private: static PbDeviceType ParseDeviceType(const string&); - // Processing flag - atomic_bool target_is_active; + mutex execution_locker; string access_token; diff --git a/cpp/piscsi/piscsi_executor.cpp b/cpp/piscsi/piscsi_executor.cpp index c5564e01..fcb43db8 100644 --- a/cpp/piscsi/piscsi_executor.cpp +++ b/cpp/piscsi/piscsi_executor.cpp @@ -10,11 +10,8 @@ #include "shared/piscsi_util.h" #include "shared/protobuf_util.h" #include "shared/piscsi_exceptions.h" -#include "controllers/scsi_controller.h" #include "devices/device_factory.h" -#include "devices/primary_device.h" #include "devices/disk.h" -#include "piscsi_image.h" #include "localizer.h" #include "command_context.h" #include "piscsi_executor.h" @@ -82,7 +79,7 @@ bool PiscsiExecutor::ProcessDeviceCmd(const CommandContext& context, const PbDev break; default: - return context.ReturnLocalizedError(LocalizationKey::ERROR_OPERATION); + return context.ReturnLocalizedError(LocalizationKey::ERROR_OPERATION, to_string(operation)); } return true; @@ -92,36 +89,20 @@ bool PiscsiExecutor::ProcessCmd(const CommandContext& context) { const PbCommand& command = context.GetCommand(); + // Handle commands that are not device-specific switch (command.operation()) { case DETACH_ALL: DetachAll(); return context.ReturnSuccessStatus(); case RESERVE_IDS: { - const string ids = GetParam(command, "ids"); - if (const string error = SetReservedIds(ids); !error.empty()) { + if (const string error = SetReservedIds(GetParam(command, "ids")); !error.empty()) { return context.ReturnErrorStatus(error); } return context.ReturnSuccessStatus(); } - case CREATE_IMAGE: - return piscsi_image.CreateImage(context); - - case DELETE_IMAGE: - return piscsi_image.DeleteImage(context); - - case RENAME_IMAGE: - return piscsi_image.RenameImage(context); - - case COPY_IMAGE: - return piscsi_image.CopyImage(context); - - case PROTECT_IMAGE: - case UNPROTECT_IMAGE: - return piscsi_image.SetImagePermissions(context); - default: // This is a device-specific command handled below break; @@ -129,10 +110,11 @@ bool PiscsiExecutor::ProcessCmd(const CommandContext& context) // Remember the list of reserved files during the dry run const auto& reserved_files = StorageDevice::GetReservedFiles(); - const bool reserved = ranges::find_if_not(context.GetCommand().devices(), [&] (const auto& device) + const bool isDryRunError = ranges::find_if_not(command.devices(), [&] (const auto& device) { return ProcessDeviceCmd(context, device, true); }) != command.devices().end(); StorageDevice::SetReservedFiles(reserved_files); - if (reserved) { + + if (isDryRunError) { return false; } @@ -145,16 +127,6 @@ bool PiscsiExecutor::ProcessCmd(const CommandContext& context) return false; } - // ATTACH and DETACH return the device list - if (command.operation() == ATTACH || command.operation() == DETACH) { - // A new command with an empty device list is required here in order to return data for all devices - PbCommand cmd; - PbResult result; - piscsi_response.GetDevicesInfo(controller_manager.GetAllDevices(), result, cmd, context.GetDefaultFolder()); - context.WriteResult(result); - return true; - } - return context.ReturnSuccessStatus(); } @@ -336,12 +308,11 @@ bool PiscsiExecutor::Insert(const CommandContext& context, const PbDeviceDefinit spdlog::info("Insert " + string(pb_device.protected_() ? "protected " : "") + "file '" + filename + "' requested into " + device->GetIdentifier()); - // TODO It may be better to add PrimaryDevice::Insert for all device-specific insert operations - auto storage_device = dynamic_pointer_cast(device); - if (!SetSectorSize(context, storage_device, pb_device.block_size())) { + if (!SetSectorSize(context, device, pb_device.block_size())) { return false; } + auto storage_device = dynamic_pointer_cast(device); if (!ValidateImageFile(context, *storage_device, filename)) { return false; } @@ -391,57 +362,6 @@ void PiscsiExecutor::DetachAll() spdlog::info("Detached all devices"); } -bool PiscsiExecutor::ShutDown(const CommandContext& context, const string& mode) { - if (mode.empty()) { - return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_MODE_MISSING); - } - - PbResult result; - result.set_status(true); - - // The PiSCSI shutdown mode is "rascsi" instead of "piscsi" for backwards compatibility - if (mode == "rascsi") { - spdlog::info("PiSCSI shutdown requested"); - - context.WriteResult(result); - - return true; - } - - if (mode != "system" && mode != "reboot") { - return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_MODE_INVALID, mode); - } - - // The root user has UID 0 - if (getuid()) { - return context.ReturnLocalizedError(LocalizationKey::ERROR_SHUTDOWN_PERMISSION); - } - - if (mode == "system") { - spdlog::info("System shutdown requested"); - - DetachAll(); - - context.WriteResult(result); - - if (system("init 0") == -1) { - spdlog::error("System shutdown failed"); - } - } - else if (mode == "reboot") { - spdlog::info("System reboot requested"); - - DetachAll(); - - context.WriteResult(result); - - if (system("init 6") == -1) { - spdlog::error("System reboot failed"); - } - } - - return false; -} string PiscsiExecutor::SetReservedIds(string_view ids) { diff --git a/cpp/piscsi/piscsi_executor.h b/cpp/piscsi/piscsi_executor.h index efa5916e..bae4fe60 100644 --- a/cpp/piscsi/piscsi_executor.h +++ b/cpp/piscsi/piscsi_executor.h @@ -11,25 +11,22 @@ #include "hal/bus.h" #include "controllers/controller_manager.h" -#include "piscsi/piscsi_response.h" #include -class PiscsiImage; class DeviceFactory; class PrimaryDevice; class StorageDevice; class CommandContext; -using namespace spdlog; - class PiscsiExecutor { public: - PiscsiExecutor(PiscsiImage& piscsi_image, BUS& bus, ControllerManager& controller_manager) - : piscsi_image(piscsi_image), bus(bus), controller_manager(controller_manager) {} + PiscsiExecutor(BUS& bus, ControllerManager& controller_manager) : bus(bus), controller_manager(controller_manager) {} ~PiscsiExecutor() = default; + // TODO At least some of these methods should be private, currently they are directly called by the unit tests + auto GetReservedIds() const { return reserved_ids; } bool ProcessDeviceCmd(const CommandContext&, const PbDeviceDefinition&, bool); @@ -43,7 +40,6 @@ public: bool Insert(const CommandContext&, const PbDeviceDefinition&, const shared_ptr&, bool) const; bool Detach(const CommandContext&, PrimaryDevice&, bool); void DetachAll(); - bool ShutDown(const CommandContext&, const string&); string SetReservedIds(string_view); bool ValidateImageFile(const CommandContext&, StorageDevice&, const string&) const; string PrintCommand(const PbCommand&, const PbDeviceDefinition&) const; @@ -60,10 +56,6 @@ private: static bool CheckForReservedFile(const CommandContext&, const string&); - const PiscsiResponse piscsi_response; - - PiscsiImage& piscsi_image; - BUS& bus; ControllerManager& controller_manager; diff --git a/cpp/piscsi/piscsi_image.cpp b/cpp/piscsi/piscsi_image.cpp index 4c7dcc42..8e569e04 100644 --- a/cpp/piscsi/piscsi_image.cpp +++ b/cpp/piscsi/piscsi_image.cpp @@ -276,6 +276,10 @@ bool PiscsiImage::SetImagePermissions(const CommandContext& context) const const bool protect = context.GetCommand().operation() == PROTECT_IMAGE; + if (protect && !IsReservedFile(context, full_filename, "protect")) { + return false; + } + try { permissions(path(full_filename), protect ? perms::owner_read | perms::group_read | perms::others_read : diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index 9b315392..fcf45cd5 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -14,15 +14,10 @@ #include "test_shared.h" #include "hal/bus.h" #include "controllers/scsi_controller.h" -#include "devices/primary_device.h" -#include "devices/storage_device.h" -#include "devices/disk.h" -#include "devices/scsihd.h" #include "devices/scsihd_nec.h" #include "devices/scsicd.h" #include "devices/scsimo.h" #include "devices/host_services.h" -#include "piscsi/command_context.h" #include "piscsi/piscsi_executor.h" #include diff --git a/cpp/test/piscsi_executor_test.cpp b/cpp/test/piscsi_executor_test.cpp index a4a917af..d6557be4 100644 --- a/cpp/test/piscsi_executor_test.cpp +++ b/cpp/test/piscsi_executor_test.cpp @@ -15,7 +15,6 @@ #include "generated/piscsi_interface.pb.h" #include "piscsi/command_context.h" #include "piscsi/piscsi_response.h" -#include "piscsi/piscsi_image.h" #include "piscsi/piscsi_executor.h" #include @@ -31,8 +30,7 @@ TEST(PiscsiExecutorTest, ProcessDeviceCmd) auto bus = make_shared(); ControllerManager controller_manager; MockAbstractController controller(bus, ID); - PiscsiImage piscsi_image; - auto executor = make_shared(piscsi_image, *bus, controller_manager); + auto executor = make_shared(*bus, controller_manager); PbDeviceDefinition definition; PbCommand command; CommandContext context(command, "", ""); @@ -115,8 +113,7 @@ TEST(PiscsiExecutorTest, ProcessCmd) auto bus = make_shared(); ControllerManager controller_manager; MockAbstractController controller(bus, 0); - PiscsiImage piscsi_image; - auto executor = make_shared(piscsi_image, *bus, controller_manager); + auto executor = make_shared(*bus, controller_manager); PbCommand command_detach_all; command_detach_all.set_operation(DETACH_ALL); @@ -167,35 +164,6 @@ TEST(PiscsiExecutorTest, ProcessCmd) device2->set_unit(1); CommandContext context_attach2(command_attach2, "", ""); EXPECT_FALSE(executor->ProcessCmd(context_attach2)) << "LUN 0 is missing"; - - // The operations below must fail because of missing parameters. - // The respective functionality is tested in piscsi_image_test.cpp. - - PbCommand command; - - command.set_operation(CREATE_IMAGE); - CommandContext context_create_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_create_image)); - - command.set_operation(DELETE_IMAGE); - CommandContext context_delete_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_delete_image)); - - command.set_operation(RENAME_IMAGE); - CommandContext context_rename_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_rename_image)); - - command.set_operation(COPY_IMAGE); - CommandContext context_copy_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_copy_image)); - - command.set_operation(PROTECT_IMAGE); - CommandContext context_protect_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_protect_image)); - - command.set_operation(UNPROTECT_IMAGE); - CommandContext context_unprotect_image(command, "", ""); - EXPECT_FALSE(executor->ProcessCmd(context_unprotect_image)); } TEST(PiscsiExecutorTest, Attach) @@ -206,8 +174,7 @@ TEST(PiscsiExecutorTest, Attach) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbDeviceDefinition definition; PbCommand command; CommandContext context(command, "", ""); @@ -290,8 +257,7 @@ TEST(PiscsiExecutorTest, Insert) auto bus = make_shared(); ControllerManager controller_manager; auto [controller, device] = CreateDevice(SCHD); - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbDeviceDefinition definition; PbCommand command; CommandContext context(command, "", ""); @@ -348,8 +314,7 @@ TEST(PiscsiExecutorTest, Detach) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -375,8 +340,7 @@ TEST(PiscsiExecutorTest, DetachAll) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); auto device = device_factory.CreateDevice(SCHS, 0, ""); EXPECT_TRUE(controller_manager.AttachToController(*bus, ID, device)); @@ -388,31 +352,12 @@ TEST(PiscsiExecutorTest, DetachAll) EXPECT_TRUE(controller_manager.GetAllDevices().empty()); } -TEST(PiscsiExecutorTest, ShutDown) -{ - auto bus = make_shared(); - ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); - - PbCommand command; - command.set_operation(SHUT_DOWN); - CommandContext context(command, "", ""); - EXPECT_FALSE(executor.ShutDown(context, "")); - EXPECT_FALSE(executor.ShutDown(context, "xyz")); - - EXPECT_FALSE(executor.ShutDown(context, "system")) << "Only available for the root user"; - EXPECT_FALSE(executor.ShutDown(context, "reboot")) << "Only available for the root user"; - EXPECT_TRUE(executor.ShutDown(context, "rascsi")); -} - TEST(PiscsiExecutorTest, SetReservedIds) { DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); string error = executor.SetReservedIds("xyz"); EXPECT_FALSE(error.empty()); @@ -451,8 +396,7 @@ TEST(PiscsiExecutorTest, ValidateImageFile) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -466,8 +410,7 @@ TEST(PiscsiExecutorTest, PrintCommand) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbDeviceDefinition definition; PbCommand command; @@ -493,8 +436,7 @@ TEST(PiscsiExecutorTest, EnsureLun0) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; auto device1 = command.add_devices(); @@ -521,8 +463,7 @@ TEST(PiscsiExecutorTest, VerifyExistingIdAndLun) DeviceFactory device_factory; auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -537,8 +478,7 @@ TEST(PiscsiExecutorTest, CreateDevice) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -555,8 +495,7 @@ TEST(PiscsiExecutorTest, SetSectorSize) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -575,8 +514,7 @@ TEST(PiscsiExecutorTest, ValidateOperationAgainstDevice) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -629,8 +567,7 @@ TEST(PiscsiExecutorTest, ValidateIdAndLun) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); @@ -646,8 +583,7 @@ TEST(PiscsiExecutorTest, SetProductData) { auto bus = make_shared(); ControllerManager controller_manager; - PiscsiImage piscsi_image; - PiscsiExecutor executor(piscsi_image, *bus, controller_manager); + PiscsiExecutor executor(*bus, controller_manager); PbCommand command; CommandContext context(command, "", ""); PbDeviceDefinition definition; diff --git a/cpp/test/piscsi_response_test.cpp b/cpp/test/piscsi_response_test.cpp index 5fe6ac1a..510c2637 100644 --- a/cpp/test/piscsi_response_test.cpp +++ b/cpp/test/piscsi_response_test.cpp @@ -17,6 +17,7 @@ #include using namespace piscsi_interface; +using namespace spdlog; using namespace protobuf_util; TEST(PiscsiResponseTest, Operation_Count) diff --git a/cpp/test/test_shared.cpp b/cpp/test/test_shared.cpp index 91e8c4f6..3ab8ad29 100644 --- a/cpp/test/test_shared.cpp +++ b/cpp/test/test_shared.cpp @@ -11,7 +11,6 @@ #include "mocks.h" #include "shared/piscsi_exceptions.h" #include "shared/piscsi_version.h" -#include #include #include #include