From b78f8573b523226b25cb436ae723ce15067bb51d Mon Sep 17 00:00:00 2001 From: Alexander Shaduri Date: Wed, 21 Apr 2021 16:48:46 +0400 Subject: [PATCH] Implemented smartctl process environment changes without changing the main program's environment. Removed ScopedEnv class - it's not thread-safe and very easy to misuse. --- src/applib/async_command_executor.cpp | 13 +++-- src/hz/env_tools.h | 77 ++------------------------- 2 files changed, 11 insertions(+), 79 deletions(-) diff --git a/src/applib/async_command_executor.cpp b/src/applib/async_command_executor.cpp index bf67017..b3ae9c7 100644 --- a/src/applib/async_command_executor.cpp +++ b/src/applib/async_command_executor.cpp @@ -25,7 +25,6 @@ Copyright: #include "hz/process_signal.h" // hz::process_signal_send, win32's W* #include "hz/debug.h" -#include "hz/env_tools.h" // hz::ScopedEnv #include "async_command_executor.h" @@ -147,7 +146,7 @@ bool AsyncCommandExecutor::execute() // Set the locale for a child to Classic - otherwise it may mangle the output. - // TODO: make this controllable. + // TODO: Disable this for JSON format. bool change_lang = true; #ifdef _WIN32 // LANG is posix-only, so it has no effect on win32. @@ -156,15 +155,19 @@ bool AsyncCommandExecutor::execute() change_lang = false; #endif - hz::ScopedEnv lang_env("LANG", "C", change_lang); - + std::unique_ptr child_env(g_get_environ(), &g_strfreev); + if (change_lang) { + child_env.reset(g_environ_setenv(child_env.release(), "LANG", "C", TRUE)); + } + std::vector envp = Glib::ArrayHandler::array_to_vector(child_env.release(), + Glib::OWNERSHIP_DEEP); debug_out_info("app", DBG_FUNC_MSG << "Executing \"" << cmd << "\".\n"); // Execute the command try { - Glib::spawn_async_with_pipes(Glib::get_current_dir(), argvp, std::vector(), + Glib::spawn_async_with_pipes(Glib::get_current_dir(), argvp, envp, Glib::SpawnFlags::SPAWN_SEARCH_PATH | Glib::SpawnFlags::SPAWN_DO_NOT_REAP_CHILD, Glib::SlotSpawnChildSetup(), &this->pid_, nullptr, &fd_stdout_, &fd_stderr_); diff --git a/src/hz/env_tools.h b/src/hz/env_tools.h index 81ed9c4..4d9a8bb 100644 --- a/src/hz/env_tools.h +++ b/src/hz/env_tools.h @@ -130,6 +130,7 @@ inline bool env_get_value(const std::string& name, std::string& value) /// Set environment variable. If \c overwrite is false, the value won't /// be overwritten if it already exists. +/// NOTE: This function is not thread-safe in GLibc and should only be invoked at the start of the program. /// \return true if the value was written successfully. inline bool env_set_value(const std::string& name, const std::string& value, bool overwrite = true) { @@ -179,7 +180,8 @@ inline bool env_set_value(const std::string& name, const std::string& value, boo -/// Unset an environment variable +/// Unset an environment variable. +/// NOTE: This function is not thread-safe in GLibc and should only be invoked at the start of the program. inline bool env_unset_value(const std::string& name) { if (name.empty() || name.find('=') != std::string::npos) @@ -213,79 +215,6 @@ inline bool env_unset_value(const std::string& name) -/// Temporarily change a value of an environment variable (for as long -/// as an object of this class exists). -class ScopedEnv { - - public: - - /// Constructor. - /// \param name variable name - /// \param value variable value to set - /// \param do_change if false, no operation will be performed. This is useful if you need - /// to conditionally set a variable (you can't practically declare a scoped variable inside - /// a conditional block to be used outside it). - /// \param overwrite if false and the variable already exists, don't change it. - ScopedEnv(std::string name, const std::string& value, bool do_change = true, bool overwrite = true) - : name_(std::move(name)), do_change_(do_change), old_set_(false), error_(false) - { - if (do_change_) { - old_set_ = env_get_value(name_, old_value_); - if (old_set_ && !overwrite) { - do_change_ = false; - } else { - error_ = !env_set_value(name_, value, true); - } - } - } - - - /// Destructor, changes back the variable to the old value - ~ScopedEnv() - { - if (do_change_) { - if (old_set_) { - env_set_value(name_, old_value_, true); - } else { - env_unset_value(name_); - } - } - } - - - /// If true, there was an error setting the value. - [[nodiscard]] bool bad() const - { - return error_; - } - - - /// Check if there was a value before we set it - [[nodiscard]] bool get_old_set() const - { - return old_set_; - } - - - /// Get the old variable value - [[nodiscard]] std::string get_old_value() const - { - return old_value_; - } - - - private: - - std::string name_; ///< Variable name - std::string old_value_; ///< Old value - bool do_change_ = false; ///< If false, don't do anything - bool old_set_ = false; ///< If false, there was no variable before we set it - bool error_ = false; ///< If true, there was an error setting the value - -}; - - - } // ns