From 74a0cf012b055a8dab83a8c8afcf82716832ee3a Mon Sep 17 00:00:00 2001 From: Alexander Shaduri Date: Mon, 20 May 2024 17:41:29 +0400 Subject: [PATCH] Refactored so that executor options are given as a vector, not as a string; this improves reliability as the string does not have to be shell-parsed. --- src/applib/async_command_executor.cpp | 30 ++++------ src/applib/async_command_executor.h | 8 +-- src/applib/command_executor.cpp | 6 +- src/applib/command_executor.h | 12 ++-- src/applib/command_executor_gui.h | 4 +- .../examples/example_smartctl_executor.cpp | 2 +- src/applib/selftest.cpp | 8 +-- src/applib/smartctl_executor.cpp | 35 ++++++------ src/applib/smartctl_executor.h | 9 +-- src/applib/storage_detector.h | 1 + src/applib/storage_detector_helpers.h | 10 ++-- src/applib/storage_detector_win32.cpp | 24 +++++--- src/applib/storage_device.cpp | 57 ++++++++++++------- src/applib/storage_device.h | 10 ++-- src/applib/storage_settings.h | 14 ++++- src/gsc_add_device_window.cpp | 14 ++++- src/gsc_executor_log_window.cpp | 16 ++++-- src/gsc_main_window.cpp | 24 +++++--- src/gsc_main_window.h | 2 +- 19 files changed, 170 insertions(+), 116 deletions(-) diff --git a/src/applib/async_command_executor.cpp b/src/applib/async_command_executor.cpp index b639320..d386688 100644 --- a/src/applib/async_command_executor.cpp +++ b/src/applib/async_command_executor.cpp @@ -111,10 +111,10 @@ AsyncCommandExecutor::~AsyncCommandExecutor() -void AsyncCommandExecutor::set_command(const std::string& command_exec, const std::string& command_args) +void AsyncCommandExecutor::set_command(std::string command_exec, std::vector command_args) { - command_exec_ = command_exec; - command_args_ = command_args; + command_exec_ = std::move(command_exec); + command_args_ = std::move(command_args); } @@ -132,21 +132,6 @@ bool AsyncCommandExecutor::execute() str_stderr_.clear(); - const std::string cmd = command_exec_ + " " + command_args_; - - - // Make command vector - std::vector argvp; - try { - argvp = Glib::shell_parse_argv(cmd); - } - catch(Glib::ShellError& e) - { - push_error(Error("gshell", ErrorLevel::Error, e.what())); - return false; - } - - // Set the locale for a child to Classic - otherwise it may mangle the output. // TODO: Disable this for JSON format. bool change_lang = true; @@ -173,7 +158,14 @@ bool AsyncCommandExecutor::execute() path_changed = !ec; } - debug_out_info("app", DBG_FUNC_MSG << "Executing \"" << cmd << "\".\n"); + debug_out_info("app", DBG_FUNC_MSG << "Executing \"" << command_exec_ << "\".\n"); + debug_out_info("app", DBG_FUNC_MSG << "Arguments:\n"); + for (const auto& arg : command_args_) { + debug_out_info("app", " " << arg << "\n"); + } + + std::vector argvp = {command_exec_}; + argvp.insert(argvp.end(), command_args_.begin(), command_args_.end()); // Execute the command try { diff --git a/src/applib/async_command_executor.h b/src/applib/async_command_executor.h index 8ff2128..5a8b68e 100644 --- a/src/applib/async_command_executor.h +++ b/src/applib/async_command_executor.h @@ -54,10 +54,8 @@ class AsyncCommandExecutor : public hz::ErrorHolder { /// Set the command to execute. Call before execute(). - /// Note: The command and the arguments _must_ be shell-escaped - /// using CommandExecutor::shell_quote(). Note that each argument - /// must be escaped separately. - void set_command(const std::string& command_exec, const std::string& command_args); + /// The spawning function escapes the command and arguments, so you don't have to. + void set_command(std::string command_exec, std::vector command_args); /// Launch the command. @@ -173,7 +171,7 @@ class AsyncCommandExecutor : public hz::ErrorHolder { // default command and its args. std::strings, not ustrings. std::string command_exec_; /// Binary name to execute. NOT affected by cleanup_members(). - std::string command_args_; /// Arguments that always go with the binary. NOT affected by cleanup_members(). + std::vector command_args_; /// Arguments that always go with the binary. NOT affected by cleanup_members(). bool running_ = false; ///< If true, the child process is running now. NOT affected by cleanup_members(). diff --git a/src/applib/command_executor.cpp b/src/applib/command_executor.cpp index 81bb362..61959e1 100644 --- a/src/applib/command_executor.cpp +++ b/src/applib/command_executor.cpp @@ -27,7 +27,7 @@ cmdex_signal_execute_finish_t& cmdex_sync_signal_execute_finish() -CommandExecutor::CommandExecutor(std::string command_name, std::string command_args) +CommandExecutor::CommandExecutor(std::string command_name, std::vector command_args) : CommandExecutor() { this->set_command(std::move(command_name), std::move(command_args)); @@ -44,7 +44,7 @@ CommandExecutor::CommandExecutor() -void CommandExecutor::set_command(std::string command_name, std::string command_args) +void CommandExecutor::set_command(std::string command_name, std::vector command_args) { cmdex_.set_command(command_name, command_args); // keep a copy locally to avoid locking on get() every time @@ -61,7 +61,7 @@ std::string CommandExecutor::get_command_name() const -std::string CommandExecutor::get_command_args() const +std::vector CommandExecutor::get_command_args() const { return command_args_; } diff --git a/src/applib/command_executor.h b/src/applib/command_executor.h index 9903157..eab751e 100644 --- a/src/applib/command_executor.h +++ b/src/applib/command_executor.h @@ -26,7 +26,7 @@ Copyright: /// Information about a finished command. struct CommandExecutorResult { - CommandExecutorResult(std::string arg_command, std::string arg_parameters, + CommandExecutorResult(std::string arg_command, std::vector arg_parameters, std::string arg_std_output, std::string arg_std_error, std::string arg_error_message) : command(std::move(arg_command)), parameters(std::move(arg_parameters)), @@ -36,7 +36,7 @@ struct CommandExecutorResult { { } const std::string command; ///< Executed command - const std::string parameters; ///< Command parameters + const std::vector parameters; ///< Command parameters const std::string std_output; ///< Stdout data const std::string std_error; ///< Stderr data const std::string error_message; ///< Execution error message @@ -63,7 +63,7 @@ class CommandExecutor : public sigc::trackable { CommandExecutor(); /// Constructor - CommandExecutor(std::string command_name, std::string command_args); + CommandExecutor(std::string command_name, std::vector command_args); /// Deleted @@ -84,7 +84,7 @@ class CommandExecutor : public sigc::trackable { /// Set command to execute and its parameters - void set_command(std::string command_name, std::string command_args); + void set_command(std::string command_name, std::vector command_args); /// Get command to execute @@ -92,7 +92,7 @@ class CommandExecutor : public sigc::trackable { /// Get command arguments - [[nodiscard]] std::string get_command_args() const; + [[nodiscard]] std::vector get_command_args() const; /// Execute the command. The function will return only after the command exits. @@ -213,7 +213,7 @@ class CommandExecutor : public sigc::trackable { AsyncCommandExecutor cmdex_; ///< Command executor std::string command_name_; ///< Command name - std::string command_args_; ///< Command arguments + std::vector command_args_; ///< Command arguments std::string running_msg_; ///< "Running" message (to show in the dialogs, etc.) diff --git a/src/applib/command_executor_gui.h b/src/applib/command_executor_gui.h index b535fd0..b7b4935 100644 --- a/src/applib/command_executor_gui.h +++ b/src/applib/command_executor_gui.h @@ -26,8 +26,8 @@ class CommandExecutorGui : public CommandExecutor { public: /// Constructor - CommandExecutorGui(const std::string& cmd, const std::string& cmdargs) - : CommandExecutor(cmd, cmdargs) + CommandExecutorGui(std::string cmd, std::vector cmdargs) + : CommandExecutor(std::move(cmd), std::move(cmdargs)) { signal_execute_tick().connect(sigc::mem_fun(*this, &CommandExecutorGui::execute_tick_func)); } diff --git a/src/applib/examples/example_smartctl_executor.cpp b/src/applib/examples/example_smartctl_executor.cpp index db8292b..7ad802f 100644 --- a/src/applib/examples/example_smartctl_executor.cpp +++ b/src/applib/examples/example_smartctl_executor.cpp @@ -34,7 +34,7 @@ int main(int argc, char** argv) // SmartctlExecutorGui ex("ls", "-l --color=no -R /dev"); // SmartctlExecutorGui ex("lsa", "-1 --color=no /sys/block"); // SmartctlExecutorGui ex("../../../0test_binary.sh", ""); - SmartctlExecutor ex("../../../0test_binary.sh", ""); + SmartctlExecutor ex("../../../0test_binary.sh", {}); ex.execute(); diff --git a/src/applib/selftest.cpp b/src/applib/selftest.cpp index 1a50db6..f8893b6 100644 --- a/src/applib/selftest.cpp +++ b/src/applib/selftest.cpp @@ -230,7 +230,7 @@ hz::ExpectedVoid SelfTest::start(const std::shared_ptrexecute_device_smartctl("--test=" + test_param, smartctl_ex, output); + auto execute_status = drive_->execute_device_smartctl({"--test=" + test_param}, smartctl_ex, output); if (!execute_status.has_value()) { std::string message = execute_status.error().message(); @@ -298,7 +298,7 @@ hz::ExpectedVoid SelfTest::force_stop(const std::shared_ // To abort non-captive short, long and conveyance tests, use "--abort". std::string output; - auto execute_status = drive_->execute_device_smartctl("--abort", smartctl_ex, output); + auto execute_status = drive_->execute_device_smartctl({"--abort"}, smartctl_ex, output); if (!execute_status) { std::string message = execute_status.error().message(); @@ -356,10 +356,10 @@ hz::ExpectedVoid SelfTest::update(const std::shared_ptr< auto parser_format = SmartctlVersionParser::get_default_format(parser_type); // ATA shows status in capabilities; NVMe shows it in self-test log. - std::string command_options = "--capabilities --log=selftest"; + std::vector command_options = {"--capabilities", "--log=selftest"}; if (parser_format == SmartctlOutputFormat::Json) { // --json flags: o means include original output (just in case). - command_options += " --json=o"; + command_options.push_back(" --json=o"); } std::string output; diff --git a/src/applib/smartctl_executor.cpp b/src/applib/smartctl_executor.cpp index a2dbc3b..9b27f3e 100644 --- a/src/applib/smartctl_executor.cpp +++ b/src/applib/smartctl_executor.cpp @@ -9,7 +9,7 @@ Copyright: /// \weakgroup applib /// @{ -#include "local_glibmm.h" // Glib::shell_quote() +#include "local_glibmm.h" #include "smartctl_executor.h" #include "hz/win32_tools.h" @@ -17,6 +17,7 @@ Copyright: #include "app_regex.h" #include "hz/fs.h" #include "build_config.h" +#include @@ -80,8 +81,8 @@ hz::fs::path get_smartctl_binary() -hz::ExpectedVoid execute_smartctl(const std::string& device, const std::string& device_opts, - const std::string& command_options, +hz::ExpectedVoid execute_smartctl(const std::string& device, const std::vector& device_opts, + const std::vector& command_options, std::shared_ptr smartctl_ex, std::string& smartctl_output) { // win32 doesn't have slashes in devices names. For others, check that slash is present. @@ -104,20 +105,22 @@ hz::ExpectedVoid execute_smartctl(const std::string& devi return hz::Unexpected(SmartctlExecutorError::NoBinary, _("Smartctl binary is not specified in configuration.")); } - auto smartctl_def_options = rconfig::get_data("system/smartctl_options"); + auto smartctl_def_options_str = hz::string_trim_copy(rconfig::get_data("system/smartctl_options")); + std::vector smartctl_options; + if (!smartctl_def_options_str.empty()) { + try { + smartctl_options = Glib::shell_parse_argv(smartctl_def_options_str); + } + catch(Glib::ShellError& e) + { + return hz::Unexpected(SmartctlExecutorError::InvalidCommandLine, _("Invalid command line specified.")); + } + } + smartctl_options.insert(smartctl_options.end(), device_opts.begin(), device_opts.end()); + smartctl_options.insert(smartctl_options.end(), command_options.begin(), command_options.end()); + smartctl_options.push_back(device); - if (!smartctl_def_options.empty()) - smartctl_def_options += " "; - - - std::string device_specific_options = device_opts; - if (!device_specific_options.empty()) - device_specific_options += " "; - - - smartctl_ex->set_command(CommandExecutor::shell_quote(hz::fs_path_to_string(smartctl_binary)), - smartctl_def_options + device_specific_options + command_options - + " " + CommandExecutor::shell_quote(device)); + smartctl_ex->set_command(hz::fs_path_to_string(smartctl_binary), smartctl_options); if (!smartctl_ex->execute() || !smartctl_ex->get_error_msg().empty()) { debug_out_warn("app", DBG_FUNC_MSG << "Smartctl binary did not execute cleanly.\n"); diff --git a/src/applib/smartctl_executor.h b/src/applib/smartctl_executor.h index 51c2a81..f080839 100644 --- a/src/applib/smartctl_executor.h +++ b/src/applib/smartctl_executor.h @@ -31,8 +31,8 @@ class SmartctlExecutorGeneric : public ExecutorSync { public: /// Constructor - SmartctlExecutorGeneric(const std::string& cmd, const std::string& cmdargs) - : ExecutorSync(cmd, cmdargs) + SmartctlExecutorGeneric(std::string cmd, std::vector cmdargs) + : ExecutorSync(std::move(cmd), std::move(cmdargs)) { this->construct(); } @@ -175,13 +175,14 @@ enum class SmartctlExecutorError { PermissionDenied, ///< Permission denied while opening device ExecutionError, ///< Error executing smartctl EmptyOutput, ///< Smartctl returned an empty output + InvalidCommandLine, ///< Invalid command line }; /// Execute smartctl on device \c device. /// \return error message on error, empty string on success. -[[nodiscard]] hz::ExpectedVoid execute_smartctl(const std::string& device, const std::string& device_opts, - const std::string& command_options, +[[nodiscard]] hz::ExpectedVoid execute_smartctl(const std::string& device, const std::vector& device_opts, + const std::vector& command_options, std::shared_ptr smartctl_ex, std::string& smartctl_output); diff --git a/src/applib/storage_detector.h b/src/applib/storage_detector.h index 84ac6bd..a88d0b8 100644 --- a/src/applib/storage_detector.h +++ b/src/applib/storage_detector.h @@ -32,6 +32,7 @@ enum class StorageDetectorError { GeneralDetectionErrors, ConfigError, DevOpenError, + InvalidCommandLine, }; diff --git a/src/applib/storage_detector_helpers.h b/src/applib/storage_detector_helpers.h index b02d5af..b152ef3 100644 --- a/src/applib/storage_detector_helpers.h +++ b/src/applib/storage_detector_helpers.h @@ -15,7 +15,7 @@ Copyright: #include #include -#include "local_glibmm.h" // Glib::shell_quote(), compose +#include "local_glibmm.h" // Glib::compose #include "build_config.h" #include "command_executor_factory.h" @@ -31,7 +31,7 @@ Copyright: /// Find and execute tw_cli with specified options, return its output through \c output. /// \return error message inline hz::ExpectedVoid execute_tw_cli(const CommandExecutorFactoryPtr& ex_factory, - const std::string& command_options, std::string& output) + const std::vector& command_options, std::string& output) { std::shared_ptr executor = ex_factory->create_executor(CommandExecutorFactory::ExecutorType::TwCli); @@ -53,7 +53,7 @@ inline hz::ExpectedVoid execute_tw_cli(const CommandExecut } for (const auto& bin : binaries) { - executor->set_command(CommandExecutor::shell_quote(bin), command_options); + executor->set_command(bin, command_options); if (!executor->execute() || !executor->get_error_msg().empty()) { debug_out_warn("app", DBG_FUNC_MSG << "Error while executing tw_cli binary.\n"); @@ -83,7 +83,7 @@ inline hz::ExpectedVoid tw_cli_get_drives(const std::strin debug_out_info("app", "Getting available 3ware drives (ports) for controller " << controller << " through tw_cli...\n"); std::string output; - auto exec_status = execute_tw_cli(ex_factory, hz::string_sprintf("/c%d show all", controller), output); + auto exec_status = execute_tw_cli(ex_factory, {hz::string_sprintf("/c%d", controller), "show", "all"}, output); if (!exec_status) { return exec_status; } @@ -125,7 +125,7 @@ inline hz::ExpectedVoid tw_cli_get_controllers( debug_out_info("app", "Getting available 3ware controllers through tw_cli...\n"); std::string output; - auto exec_status = execute_tw_cli(ex_factory, "show", output); + auto exec_status = execute_tw_cli(ex_factory, {"show"}, output); if (!exec_status) { return exec_status; } diff --git a/src/applib/storage_detector_win32.cpp b/src/applib/storage_detector_win32.cpp index fc4ce77..72e0b3e 100644 --- a/src/applib/storage_detector_win32.cpp +++ b/src/applib/storage_detector_win32.cpp @@ -201,13 +201,19 @@ hz::ExpectedVoid get_scan_open_multiport_devices(std::vect return hz::Unexpected(StorageDetectorError::NoSmartctlBinary, _("Smartctl binary is not specified in configuration.")); } - auto smartctl_def_options = rconfig::get_data("system/smartctl_options"); - if (!smartctl_def_options.empty()) - smartctl_def_options += " "; - - smartctl_ex->set_command(CommandExecutor::shell_quote(hz::fs_path_to_string(smartctl_binary)), - smartctl_def_options + "--scan-open"); + auto smartctl_def_options_str = rconfig::get_data("system/smartctl_options"); + std::vector smartctl_options; + if (!smartctl_def_options_str.empty()) { + try { + smartctl_options = Glib::shell_parse_argv(smartctl_def_options_str); + } + catch(Glib::ShellError& e) + { + return hz::Unexpected(StorageDetectorError::InvalidCommandLine, _("Invalid command line specified.")); + } + } + smartctl_options.push_back("--scan-open"); if (bool execute_status = smartctl_ex->execute(); !execute_status) { debug_out_warn("app", DBG_FUNC_MSG << "Smartctl binary did not execute cleanly.\n"); @@ -276,11 +282,11 @@ hz::ExpectedVoid get_scan_open_multiport_devices(std::vect /// Find and execute areca cli with specified options, return its output through \c output. /// \return error message inline hz::ExpectedVoid execute_areca_cli(const CommandExecutorFactoryPtr& ex_factory, const std::string& cli_binary, - const std::string& command_options, std::string& output) + const std::vector& command_options, std::string& output) { std::shared_ptr executor = ex_factory->create_executor(CommandExecutorFactory::ExecutorType::ArecaCli); - executor->set_command(CommandExecutor::shell_quote(cli_binary), command_options); + executor->set_command(cli_binary, command_options); if (!executor->execute() || !executor->get_error_msg().empty()) { debug_out_warn("app", DBG_FUNC_MSG << "Error while executing Areca cli binary.\n"); @@ -392,7 +398,7 @@ GuiErrMsg<0x00>: Success. // the interactive mode. std::string output; - auto execute_status = execute_areca_cli(ex_factory, cli_binary, "disk info", output); + auto execute_status = execute_areca_cli(ex_factory, cli_binary, {"disk", "info"}, output); if (!execute_status) { return execute_status; } diff --git a/src/applib/storage_device.cpp b/src/applib/storage_device.cpp index cf4fabb..0f6d925 100644 --- a/src/applib/storage_device.cpp +++ b/src/applib/storage_device.cpp @@ -112,10 +112,10 @@ hz::ExpectedVoid StorageDevice::fetch_basic_data_and_parse( // We don't use "--all" - it may cause really screwed up the output (tests, etc.). // This looks just like "--info" only on non-smart devices. const auto default_parser_type = SmartctlVersionParser::get_default_format(SmartctlParserType::Basic); - std::string command_options = "--info --health --capabilities"; + std::vector command_options = {"--info", "--health", "--capabilities"}; if (default_parser_type == SmartctlOutputFormat::Json) { // --json flags: o means include original output (just in case). - command_options += " --json=o"; + command_options.push_back("--json=o"); } auto execute_status = execute_device_smartctl(command_options, smartctl_ex, this->basic_output_, true); // set type to invalid if needed @@ -223,7 +223,7 @@ hz::ExpectedVoid StorageDevice::fetch_full_data_and_parse( // Instead of -x, we use all the individual options -x encompasses, so that // an addition to default -x output won't affect us. - std::string command_options; + std::vector command_options; // Type was detected by Basic parser switch (this->get_detected_type()) { @@ -234,19 +234,34 @@ hz::ExpectedVoid StorageDevice::fetch_full_data_and_parse( case StorageDeviceDetectedType::AtaAny: case StorageDeviceDetectedType::AtaHdd: case StorageDeviceDetectedType::AtaSsd: - command_options = "--health --info --get=all --capabilities --attributes --format=brief --log=xerror,50,error --log=xselftest,50,selftest --log=selective --log=directory --log=scttemp --log=scterc --log=devstat --log=sataphy"; + command_options = { + "--health", + "--info", + "--get=all", + "--capabilities", + "--attributes", + "--format=brief", + "--log=xerror,50,error", + "--log=xselftest,50,selftest", + "--log=selective", + "--log=directory", + "--log=scttemp", + "--log=scterc", + "--log=devstat", + "--log=sataphy", + }; break; case StorageDeviceDetectedType::Nvme: // We don't care if something is added to json output. // Same as: --health --info --capabilities --attributes --log=error --log=selftest - command_options = "--xall"; + command_options = {"--xall"}; break; case StorageDeviceDetectedType::BasicScsi: case StorageDeviceDetectedType::CdDvd: case StorageDeviceDetectedType::UnsupportedRaid: // SCSI equivalent of -x: // command_options = "--health --info --attributes --log=error --log=selftest --log=background --log=sasphy"; - command_options = "--xall"; + command_options = {"--xall"}; break; } @@ -254,7 +269,7 @@ hz::ExpectedVoid StorageDevice::fetch_full_data_and_parse( auto parser_format = SmartctlVersionParser::get_default_format(parser_type); if (parser_format == SmartctlOutputFormat::Json) { // --json flags: o means include original output (just in case). - command_options += " --json=o"; + command_options.push_back("--json=o"); } std::string output; @@ -507,7 +522,9 @@ A mandatory SMART command failed: exiting. To continue, add one or more '-T perm */ std::string output; - auto status = execute_device_smartctl((b ? "--smart=on --saveauto=on" : "--smart=off"), smartctl_ex, output); + std::vector on_command = {"--smart=on", "--saveauto=on"}; + std::vector off_command = {"--smart=off"}; + auto status = execute_device_smartctl((b ? on_command : off_command), smartctl_ex, output); if (!status) { return status; } @@ -746,14 +763,14 @@ std::string StorageDevice::get_type_argument() const -void StorageDevice::set_extra_arguments(std::string args) +void StorageDevice::set_extra_arguments(std::vector args) { extra_args_ = std::move(args); } -std::string StorageDevice::get_extra_arguments() const +std::vector StorageDevice::get_extra_arguments() const { return extra_args_; } @@ -914,7 +931,7 @@ std::string StorageDevice::get_save_filename() const -std::string StorageDevice::get_device_options() const +std::vector StorageDevice::get_device_options() const { if (is_virtual_) { debug_out_warn("app", DBG_FUNC_MSG << "Cannot get device options of a virtual device.\n"); @@ -927,25 +944,23 @@ std::string StorageDevice::get_device_options() const // lowest priority - the detected type std::vector args; if (!get_type_argument().empty()) { - args.push_back("-d " + get_type_argument()); + args.push_back("-d"); + args.push_back(get_type_argument()); } // extra args, as specified manually in CLI or when adding the drive - if (!get_extra_arguments().empty()) { - args.push_back(get_extra_arguments()); - } + auto extra_args = get_extra_arguments(); + args.insert(args.end(), extra_args.begin(), extra_args.end()); // config options, as specified in preferences. - std::string config_options = app_get_device_option(get_device(), get_type_argument()); - if (!config_options.empty()) { - args.push_back(config_options); - } + std::vector config_options = app_get_device_options(get_device(), get_type_argument()); + args.insert(args.end(), config_options.begin(), config_options.end()); - return hz::string_join(args, " "); + return args; } -hz::ExpectedVoid StorageDevice::execute_device_smartctl(const std::string& command_options, +hz::ExpectedVoid StorageDevice::execute_device_smartctl(const std::vector& command_options, const std::shared_ptr& smartctl_ex, std::string& smartctl_output, bool check_type) { // don't forbid running on currently tested drive - we need to call this from the test code. diff --git a/src/applib/storage_device.h b/src/applib/storage_device.h index ffcbdae..1246275 100644 --- a/src/applib/storage_device.h +++ b/src/applib/storage_device.h @@ -161,10 +161,10 @@ class StorageDevice { /// Set extra arguments smartctl - void set_extra_arguments(std::string args); + void set_extra_arguments(std::vector args); /// Get extra arguments smartctl - [[nodiscard]] std::string get_extra_arguments() const; + [[nodiscard]] std::vector get_extra_arguments() const; /// Set windows drive letters for this drive @@ -237,11 +237,11 @@ class StorageDevice { /// Get final smartctl options for this device from config and type info. - [[nodiscard]] std::string get_device_options() const; + [[nodiscard]] std::vector get_device_options() const; /// Execute smartctl on this device. Nothing is modified in this class. - [[nodiscard]] hz::ExpectedVoid execute_device_smartctl(const std::string& command_options, + [[nodiscard]] hz::ExpectedVoid execute_device_smartctl(const std::vector& command_options, const std::shared_ptr& smartctl_ex, std::string& output, bool check_type = false); @@ -262,7 +262,7 @@ class StorageDevice { std::string device_; ///< e.g. /dev/sda or pd0. empty if virtual. std::string type_arg_; ///< Device type (for -d smartctl parameter), as specified when adding the device. - std::string extra_args_; ///< Extra parameters for smartctl, as specified when adding the device. + std::vector extra_args_; ///< Extra parameters for smartctl, as specified when adding the device. std::map drive_letters_; ///< Windows drive letters (if detected), with volume names diff --git a/src/applib/storage_settings.h b/src/applib/storage_settings.h index f4520d2..c7b91da 100644 --- a/src/applib/storage_settings.h +++ b/src/applib/storage_settings.h @@ -84,14 +84,24 @@ inline AppDeviceOptionMap app_config_get_device_option_map() /// Read device option map from config and get the options for (dev, type_arg) pair. -inline std::string app_get_device_option(const std::string& dev, const std::string& type_arg) +inline std::vector app_get_device_options(const std::string& dev, const std::string& type_arg) { if (dev.empty()) return {}; auto devmap = app_config_get_device_option_map().value; if (auto iter = devmap.find(std::pair(dev, type_arg)); iter != devmap.end()) { - return iter->second; + if (iter->second.empty()) { + return {}; + } + try { + return Glib::shell_parse_argv(iter->second); + } + catch(Glib::ShellError& e) + { + // TODO report error + return {}; + } } return {}; } diff --git a/src/gsc_add_device_window.cpp b/src/gsc_add_device_window.cpp index 082ca03..f9b5731 100644 --- a/src/gsc_add_device_window.cpp +++ b/src/gsc_add_device_window.cpp @@ -146,7 +146,8 @@ void GscAddDeviceWindow::on_window_cancel_button_clicked() void GscAddDeviceWindow::on_window_ok_button_clicked() { - std::string dev, type, params; + std::string dev, type; + std::vector params; if (auto* entry = lookup_widget("device_name_entry")) { dev = entry->get_text(); } @@ -154,7 +155,16 @@ void GscAddDeviceWindow::on_window_ok_button_clicked() type = type_combo->get_entry_text(); } if (auto* entry = lookup_widget("smartctl_params_entry")) { - params = entry->get_text(); + auto params_str = entry->get_text(); + if (!params_str.empty()) { + try { + params = Glib::shell_parse_argv(params_str); + } + catch(Glib::ShellError& e) + { + // TODO Alert + } + } } if (main_window_ && !dev.empty()) { main_window_->add_device(dev, type, params); diff --git a/src/gsc_executor_log_window.cpp b/src/gsc_executor_log_window.cpp index 1554a42..db80928 100644 --- a/src/gsc_executor_log_window.cpp +++ b/src/gsc_executor_log_window.cpp @@ -9,12 +9,14 @@ Copyright: /// \weakgroup gsc /// @{ +#include "hz/string_algo.h" #include "local_glibmm.h" #include #include // GDK_KEY_Escape #include #include // std::size_t #include +#include #include "applib/app_gtkmm_tools.h" // app_gtkmm_create_tree_view_column #include "hz/fs.h" @@ -152,10 +154,13 @@ void GscExecutorLogWindow::on_command_output_received(const CommandExecutorResul auto entry = std::make_shared(info); entries.push_back(entry); + std::vector command = {info.command}; + command.insert(command.end(), info.parameters.begin(), info.parameters.end()); + // update tree model const Gtk::TreeRow row = *(list_store->append()); row[col_num] = entries.size(); - row[col_command] = info.command + " " + info.parameters; + row[col_command] = hz::string_join(command, " "); row[col_entry] = entry; // if visible, set the selection to it @@ -299,7 +304,9 @@ void GscExecutorLogWindow::on_window_save_all_button_clicked() exss << "\n---------------" << "Command" << "---------------\n"; exss << entries[i]->command << "\n"; exss << "\n---------------" << "Parameters" << "---------------\n"; - exss << entries[i]->parameters << "\n"; + for (const auto& param : entries[i]->parameters) { + exss << param << "\n"; + } exss << "\n---------------" << "STDOUT" << "---------------\n"; exss << entries[i]->std_output << "\n\n"; exss << "\n---------------" << "STDERR" << "---------------\n"; @@ -436,8 +443,9 @@ void GscExecutorLogWindow::on_tree_selection_changed() } if (auto* command_entry = this->lookup_widget("command_entry")) { - const std::string cmd_text = entry->command + " " + entry->parameters; - command_entry->set_text(app_make_valid_utf8_from_command_output(cmd_text)); + std::vector command = {entry->command}; + command.insert(command.end(), entry->parameters.begin(), entry->parameters.end()); + command_entry->set_text(app_make_valid_utf8_from_command_output(hz::string_join(command, " "))); } if (auto* window_save_current_button = this->lookup_widget("window_save_current_button")) diff --git a/src/gsc_main_window.cpp b/src/gsc_main_window.cpp index f01bedf..7a0fcce 100644 --- a/src/gsc_main_window.cpp +++ b/src/gsc_main_window.cpp @@ -104,8 +104,7 @@ GscMainWindow::GscMainWindow(BaseObjectType* gtkcobj, Glib::RefPtr ex.create_running_dialog(this); ex.set_running_msg(_("Checking if smartctl is executable...")); -// ex.set_command(CommandExecutor::shell_quote(smartctl_binary), smartctl_def_options + "-V"); // --version - ex.set_command(CommandExecutor::shell_quote(smartctl_binary), "-V"); // --version + ex.set_command(smartctl_binary, {"-V"}); // --version if (!ex.execute() || !ex.get_error_msg().empty()) { error_msg = ex.get_error_msg(); @@ -188,9 +187,20 @@ void GscMainWindow::populate_iconview(bool smartctl_valid) if (!dev_with_type.empty()) { std::vector parts; hz::string_split(dev_with_type, "::", parts, false); - std::string file = (!parts.empty() ? parts.at(0) : std::string()); - std::string type_arg = (parts.size() > 1 ? parts.at(1) : std::string()); - std::string extra_args = (parts.size() > 2 ? parts.at(2) : std::string()); + const std::string file = (!parts.empty() ? parts.at(0) : std::string()); + const std::string type_arg = (parts.size() > 1 ? parts.at(1) : std::string()); + + const std::string extra_args_str = (parts.size() > 2 ? parts.at(2) : std::string()); + std::vector extra_args; + if (!extra_args_str.empty()) { + try { + extra_args = Glib::shell_parse_argv(extra_args_str); + } + catch(Glib::ShellError& e) + { + // TODO Report + } + } if (!file.empty()) { add_device(file, type_arg, extra_args); } @@ -946,7 +956,7 @@ void GscMainWindow::run_update_drivedb() if (smartctl_binary.is_absolute()) { update_binary_path = smartctl_binary.parent_path() / update_binary_path; } - std::string update_binary = CommandExecutor::shell_quote(hz::fs_path_to_string(update_binary_path)); + std::string update_binary = hz::fs_path_to_string(update_binary_path); if constexpr(!BuildEnv::is_kernel_family_windows()) { // X11 update_binary = "xterm -hold -e " + update_binary; @@ -962,7 +972,7 @@ void GscMainWindow::run_update_drivedb() -bool GscMainWindow::add_device(const std::string& file, const std::string& type_arg, const std::string& extra_args) +bool GscMainWindow::add_device(const std::string& file, const std::string& type_arg, const std::vector& extra_args) { // win32 doesn't have device files, so skip the check in Windows. if constexpr(!BuildEnv::is_kernel_family_windows()) { diff --git a/src/gsc_main_window.h b/src/gsc_main_window.h index 66732f7..769e8cb 100644 --- a/src/gsc_main_window.h +++ b/src/gsc_main_window.h @@ -53,7 +53,7 @@ class GscMainWindow : public AppBuilderWidget { /// Manually add device file to icon list - bool add_device(const std::string& file, const std::string& type_arg, const std::string& extra_args); + bool add_device(const std::string& file, const std::string& type_arg, const std::vector& extra_args); /// Read smartctl data from file, add it as a virtual drive to icon list