From cba58aeabab83ae199450aa14dcd30d9961b3fe8 Mon Sep 17 00:00:00 2001 From: Arvin Schnell Date: Fri, 13 Oct 2023 17:35:01 +0200 Subject: [PATCH] - make SystemCmd take vector of strings --- LIBVERSION | 2 +- client/misc.cc | 2 +- package/snapper.changes | 1 + snapper/Ext4.cc | 14 +++--- snapper/Hooks.cc | 13 +++-- snapper/Lvm.cc | 4 +- snapper/Lvm.h | 2 +- snapper/LvmCache.cc | 24 ++++++---- snapper/Snapper.cc | 4 +- snapper/SystemCmd.cc | 103 +++++++++++++++++++++++++--------------- snapper/SystemCmd.h | 91 ++++++++++++++++++++++++++++------- snapper/Systemctl.cc | 8 +++- 12 files changed, 180 insertions(+), 88 deletions(-) diff --git a/LIBVERSION b/LIBVERSION index 0ee843cc..15020207 100644 --- a/LIBVERSION +++ b/LIBVERSION @@ -1 +1 @@ -7.2.0 +7.3.0 diff --git a/client/misc.cc b/client/misc.cc index 105049bb..0dfd222a 100644 --- a/client/misc.cc +++ b/client/misc.cc @@ -205,7 +205,7 @@ Differ::run(const string& f1, const string& f2) const tmp += " " + extensions; tmp += " " + quote(f1) + " " + quote(f2); - SystemCmd cmd(tmp); + SystemCmd cmd({ SH_BIN, "-c", tmp }); for (const string& line : cmd.get_stdout()) cout << line << endl; diff --git a/package/snapper.changes b/package/snapper.changes index 98bf49a8..11210654 100644 --- a/package/snapper.changes +++ b/package/snapper.changes @@ -2,6 +2,7 @@ Fri Oct 13 08:56:18 CEST 2023 - aschnell@suse.com - fix diff for lvm based configs (bsc#1216191) +- make SystemCmd take vector of strings ------------------------------------------------------------------- Thu Sep 14 15:45:53 CEST 2023 - aschnell@suse.com diff --git a/snapper/Ext4.cc b/snapper/Ext4.cc index f0046af5..c9827b24 100644 --- a/snapper/Ext4.cc +++ b/snapper/Ext4.cc @@ -91,7 +91,7 @@ namespace snapper int r1 = mkdir((subvolume + "/.snapshots").c_str(), 0700); if (r1 == 0) { - SystemCmd cmd1(CHATTRBIN " +x " + quote(subvolume + "/.snapshots")); + SystemCmd cmd1({ CHATTRBIN, "+x", subvolume + "/.snapshots" }); if (cmd1.retcode() != 0) throw CreateConfigFailedException("chattr failed"); } @@ -104,7 +104,7 @@ namespace snapper int r2 = mkdir((subvolume + "/.snapshots/.info").c_str(), 0700); if (r2 == 0) { - SystemCmd cmd2(CHATTRBIN " -x " + quote(subvolume + "/.snapshots/.info")); + SystemCmd cmd2({ CHATTRBIN, "-x", subvolume + "/.snapshots/.info" }); if (cmd2.retcode() != 0) throw CreateConfigFailedException("chattr failed"); } @@ -174,11 +174,11 @@ namespace snapper if (num_parent != 0 || !read_only) throw std::logic_error("not implemented"); - SystemCmd cmd1(TOUCHBIN " " + quote(snapshotFile(num))); + SystemCmd cmd1({ TOUCHBIN, snapshotFile(num) }); if (cmd1.retcode() != 0) throw CreateSnapshotFailedException(); - SystemCmd cmd2(CHSNAPBIN " +S " + quote(snapshotFile(num))); + SystemCmd cmd2({ CHSNAPBIN, "+S", snapshotFile(num) }); if (cmd2.retcode() != 0) throw CreateSnapshotFailedException(); } @@ -187,7 +187,7 @@ namespace snapper void Ext4::deleteSnapshot(unsigned int num) const { - SystemCmd cmd(CHSNAPBIN " -S " + quote(snapshotFile(num))); + SystemCmd cmd({ CHSNAPBIN, "-S", snapshotFile(num) }); if (cmd.retcode() != 0) throw DeleteSnapshotFailedException(); } @@ -212,7 +212,7 @@ namespace snapper if (isSnapshotMounted(num)) return; - SystemCmd cmd1(CHSNAPBIN " +n " + quote(snapshotFile(num))); + SystemCmd cmd1({ CHSNAPBIN, "+n", snapshotFile(num) }); if (cmd1.retcode() != 0) throw MountSnapshotFailedException(); @@ -237,7 +237,7 @@ namespace snapper // if (!umount(snapshotDir(num))) // throw UmountSnapshotFailedException(); - SystemCmd cmd1(CHSNAPBIN " -n " + quote(snapshotFile(num))); + SystemCmd cmd1({ CHSNAPBIN, "-n", snapshotFile(num) }); if (cmd1.retcode() != 0) throw UmountSnapshotFailedException(); diff --git a/snapper/Hooks.cc b/snapper/Hooks.cc index 1c98fa40..a2ddd089 100644 --- a/snapper/Hooks.cc +++ b/snapper/Hooks.cc @@ -1,6 +1,6 @@ /* * Copyright (c) [2011-2015] Novell, Inc. - * Copyright (c) 2022 SUSE LLC + * Copyright (c) [2022-2023] SUSE LLC * * All Rights Reserved. * @@ -53,10 +53,9 @@ namespace snapper std::sort(scripts.begin(), scripts.end()); for (const string& script : scripts) { - string cmd_line = dir.fullname(script); - for (const string& arg : args) - cmd_line += " " + quote(arg); - SystemCmd cmd(cmd_line); + SystemCmd::Args cmd_args = { dir.fullname(script) }; + cmd_args << args; + SystemCmd cmd(cmd_args); } } catch (const Exception& e) @@ -185,7 +184,7 @@ namespace snapper if (subvolume == "/" && filesystem->fstype() == "btrfs" && access(GRUB_SCRIPT, X_OK) == 0) { - SystemCmd cmd(string(GRUB_SCRIPT) + " " + option); + SystemCmd cmd({ GRUB_SCRIPT, option }); } #endif } @@ -201,7 +200,7 @@ namespace snapper // Fate#319108 if (access(ROLLBACK_SCRIPT, X_OK) == 0) { - SystemCmd cmd(string(ROLLBACK_SCRIPT) + " " + old_root + " " + new_root); + SystemCmd cmd({ ROLLBACK_SCRIPT, old_root, new_root }); } #endif } diff --git a/snapper/Lvm.cc b/snapper/Lvm.cc index 1ae70556..dd89289d 100644 --- a/snapper/Lvm.cc +++ b/snapper/Lvm.cc @@ -483,7 +483,7 @@ namespace snapper LvmCapabilities::LvmCapabilities() { - SystemCmd cmd(string(LVMBIN " version")); + SystemCmd cmd({ LVMBIN, "version" }); if (cmd.retcode() != 0 || cmd.get_stdout().empty()) { @@ -510,7 +510,7 @@ namespace snapper lvm_version version(maj, min, rev); if (version >= lvm_version(2, 2, 99)) - ignoreactivationskip = " -K"; + ignoreactivationskip = "--ignoreactivationskip"; } } } diff --git a/snapper/Lvm.h b/snapper/Lvm.h index bc029b02..e67009c7 100644 --- a/snapper/Lvm.h +++ b/snapper/Lvm.h @@ -72,7 +72,7 @@ namespace snapper LvmCapabilities(); - // empty or " -K" if lvm supports ignore activation skip flag + // empty or "--ignoreactivationskip" if lvm supports ignore activation skip flag string ignoreactivationskip; }; diff --git a/snapper/LvmCache.cc b/snapper/LvmCache.cc index 34bd1a0b..b34c2bd7 100644 --- a/snapper/LvmCache.cc +++ b/snapper/LvmCache.cc @@ -91,7 +91,12 @@ namespace snapper { boost::upgrade_to_unique_lock unique_lock(upg_lock); - SystemCmd cmd(LVCHANGEBIN + caps->get_ignoreactivationskip() + " -ay " + quote(full_name())); + SystemCmd::Args cmd_args = { LVCHANGEBIN }; + if (!caps->get_ignoreactivationskip().empty()) + cmd_args << caps->get_ignoreactivationskip(); + cmd_args << "--activate" << "y" << full_name(); + + SystemCmd cmd(cmd_args); if (cmd.retcode() != 0) { y2err("lvm cache: " << full_name() << " activation failed!"); @@ -120,7 +125,7 @@ namespace snapper { boost::upgrade_to_unique_lock unique_lock(upg_lock); - SystemCmd cmd(LVCHANGEBIN " -an " + quote(full_name())); + SystemCmd cmd({ LVCHANGEBIN, "--activate", "n", full_name() }); if (cmd.retcode() != 0) { y2err("lvm cache: " << full_name() << " deactivation failed!"); @@ -139,7 +144,7 @@ namespace snapper { boost::unique_lock unique_lock(lv_mutex); - SystemCmd cmd(LVSBIN " --noheadings -o lv_attr,segtype " + quote(full_name())); + SystemCmd cmd({ LVSBIN, "--noheadings", "--options", "lv_attr,segtype", full_name() }); if (cmd.retcode() != 0 || cmd.get_stdout().empty()) { y2err("lvm cache: failed to get info about " << full_name()); @@ -181,8 +186,7 @@ namespace snapper { boost::upgrade_to_unique_lock unique_lock(upg_lock); - SystemCmd cmd(LVCHANGEBIN " --permission " + string(read_only ? "r" : "rw") + " " + - quote(full_name())); + SystemCmd cmd({ LVCHANGEBIN, "--permission", read_only ? "r" : "rw", full_name() }); if (cmd.retcode() != 0) { y2err("lvm cache: " << full_name() << " setting permission failed!"); @@ -335,8 +339,8 @@ namespace snapper boost::upgrade_to_unique_lock unique_lock(upg_lock); - SystemCmd cmd(LVCREATEBIN " --permission " + string(read_only ? "r" : "rw") + " --snapshot " - "--name " + quote(lv_snapshot_name) + " " + quote(full_name(lv_origin_name))); + SystemCmd cmd({ LVCREATEBIN, "--permission", read_only ? "r" : "rw", "--snapshot", + "--name", lv_snapshot_name, full_name(lv_origin_name) }); if (cmd.retcode() != 0) throw LvmCacheException(); @@ -359,7 +363,7 @@ namespace snapper } else { - SystemCmd cmd(LVSBIN " --noheadings -o lv_attr,segtype " + quote(full_name(lv_name))); + SystemCmd cmd({ LVSBIN, "--noheadings", "--options", "lv_attr,segtype", full_name(lv_name) }); if (cmd.retcode() != 0 || cmd.get_stdout().empty()) { y2err("lvm cache: failed to get info about " << full_name(lv_name)); @@ -395,7 +399,7 @@ namespace snapper // wait for all invidual lv cache operations under shared vg lock to finish boost::upgrade_to_unique_lock unique_lock(upg_lock); - SystemCmd cmd(LVREMOVEBIN " --force " + quote(full_name(lv_name))); + SystemCmd cmd({ LVREMOVEBIN, "--force", full_name(lv_name) }); if (cmd.retcode() != 0) throw LvmCacheException(); @@ -548,7 +552,7 @@ namespace snapper void LvmCache::add_vg(const string& vg_name, const string& include_lv_name) { - SystemCmd cmd(LVSBIN " --noheadings -o lv_name,lv_attr,segtype " + quote(vg_name)); + SystemCmd cmd({ LVSBIN, "--noheadings", "--options", "lv_name,lv_attr,segtype", vg_name }); if (cmd.retcode() != 0) { y2err("lvm cache: failed to get info about VG " << vg_name); diff --git a/snapper/Snapper.cc b/snapper/Snapper.cc index 8375178e..edbe585c 100644 --- a/snapper/Snapper.cc +++ b/snapper/Snapper.cc @@ -453,7 +453,7 @@ namespace snapper sysconfig.save(); - SystemCmd cmd(RMBIN " " + quote(CONFIGS_DIR "/" + config_name)); + SystemCmd cmd({ RMBIN, "--", CONFIGS_DIR "/" + config_name }); SN_RETHROW(e); } @@ -512,7 +512,7 @@ namespace snapper SN_THROW(DeleteConfigFailedException("deleting snapshot failed")); } - SystemCmd cmd1(RMBIN " " + quote(CONFIGS_DIR "/" + config_name)); + SystemCmd cmd1({ RMBIN, "--", CONFIGS_DIR "/" + config_name }); if (cmd1.retcode() != 0) { SN_THROW(DeleteConfigFailedException("deleting config-file failed")); diff --git a/snapper/SystemCmd.cc b/snapper/SystemCmd.cc index 712daa3f..68982c8c 100644 --- a/snapper/SystemCmd.cc +++ b/snapper/SystemCmd.cc @@ -1,6 +1,6 @@ /* * Copyright (c) [2004-2011] Novell, Inc. - * Copyright (c) [2018-2021] SUSE LLC + * Copyright (c) [2018-2023] SUSE LLC * * All Rights Reserved. * @@ -21,12 +21,11 @@ */ -#include +#include #include -#include +#include #include #include -#include #include extern char **environ; @@ -35,6 +34,7 @@ extern char **environ; #include "snapper/AppUtil.h" #include "snapper/SystemCmd.h" #include "snapper/SnapperDefines.h" +#include "snapper/Exception.h" namespace snapper @@ -42,13 +42,24 @@ namespace snapper using namespace std; - SystemCmd::SystemCmd(const string& Command_Cv, bool log_output) - : log_output(log_output) -{ - y2mil("constructor SystemCmd:\"" << Command_Cv << "\""); - init(); - execute( Command_Cv ); -} + SystemCmd::SystemCmd(const Args& args, bool log_output) + : args(args), log_output(log_output) + { + y2mil("constructor SystemCmd: " << cmd()); + + if (args.get_values().empty()) + SN_THROW(Exception("args empty")); + + init(); + execute(); + } + + + string + SystemCmd::cmd() const + { + return boost::join(args.get_values(), " "); + } void SystemCmd::init() @@ -78,20 +89,9 @@ SystemCmd::closeOpenFds() const } -int -SystemCmd::execute(const string& Cmd_Cv) -{ - y2mil("SystemCmd Executing:\"" << Cmd_Cv << "\""); - return doExecute(Cmd_Cv); -} - - -int -SystemCmd::doExecute( const string& Cmd ) + void + SystemCmd::execute() { - lastCmd = Cmd; - y2deb("Cmd:" << Cmd); - StopWatch stopwatch; File_aC[IDX_STDERR] = File_aC[IDX_STDOUT] = NULL; @@ -123,7 +123,8 @@ SystemCmd::doExecute( const string& Cmd ) } y2deb("sout:" << pfds[0].fd << " serr:" << pfds[1].fd); - const vector env = make_env(); + const TmpForExec args_p(make_args()); + const TmpForExec env_p(make_env()); switch( (Pid_i=fork()) ) { @@ -144,9 +145,11 @@ SystemCmd::doExecute( const string& Cmd ) { y2err("close child failed errno:" << errno << " (" << stringerror(errno) << ")"); } + closeOpenFds(); - Ret_i = execle(SH_BIN, SH_BIN, "-c", Cmd.c_str(), nullptr, &env[0]); - y2err("SHOULD NOT HAPPEN \"" SH_BIN "\" Ret:" << Ret_i); + + Ret_i = execvpe(args.get_values()[0].c_str(), args_p.get(), env_p.get()); + y2err("SHOULD NOT HAPPEN execvpe returned ret=" << Ret_i << " errno=" << errno); break; case -1: Ret_i = -1; @@ -184,13 +187,12 @@ SystemCmd::doExecute( const string& Cmd ) } if( Ret_i==-127 || Ret_i==-1 ) { - y2err("system (\"" << Cmd << "\") = " << Ret_i); + y2err("system (\"" << cmd() << "\") = " << Ret_i); } checkOutput(); y2mil("system() Returns:" << Ret_i); if (Ret_i != 0 && log_output) logOutput(); - return Ret_i; } @@ -230,14 +232,14 @@ SystemCmd::doWait( int& Ret_ir ) { Ret_ir = WEXITSTATUS(Status_ii); if (Ret_ir == 126) - y2err("command \"" << lastCmd << "\" not executable"); + y2err("command \"" << cmd() << "\" not executable"); else if (Ret_ir == 127) - y2err("command \"" << lastCmd << "\" not found"); + y2err("command \"" << cmd() << "\" not found"); } else { Ret_ir = -127; - y2err("command \"" << lastCmd << "\" failed"); + y2err("command \"" << cmd() << "\" failed"); } } @@ -435,24 +437,47 @@ SystemCmd::logOutput() const } - vector + SystemCmd::TmpForExec::~TmpForExec() + { + for (char* v : values) + free(v); + } + + + SystemCmd::TmpForExec + SystemCmd::make_args() const + { + vector ret; + + for (const string& v : args.get_values()) + { + ret.push_back(strdup(v.c_str())); + } + + ret.push_back(nullptr); + + return ret; + } + + + SystemCmd::TmpForExec SystemCmd::make_env() const { - vector env; + vector ret; for (char** v = environ; *v != NULL; ++v) { if (strncmp(*v, "LC_ALL=", strlen("LC_ALL=")) != 0 && strncmp(*v, "LANGUAGE=", strlen("LANGUAGE=")) != 0) - env.push_back(*v); + ret.push_back(strdup(*v)); } - env.push_back("LC_ALL=C"); - env.push_back("LANGUAGE=C"); + ret.push_back(strdup("LC_ALL=C")); + ret.push_back(strdup("LANGUAGE=C")); - env.push_back(nullptr); + ret.push_back(nullptr); - return env; + return ret; } diff --git a/snapper/SystemCmd.h b/snapper/SystemCmd.h index 7067fbd5..88172b73 100644 --- a/snapper/SystemCmd.h +++ b/snapper/SystemCmd.h @@ -1,6 +1,6 @@ /* * Copyright (c) [2004-2014] Novell, Inc. - * Copyright (c) [2018-2021] SUSE LLC + * Copyright (c) [2018-2023] SUSE LLC * * All Rights Reserved. * @@ -25,10 +25,10 @@ #define SNAPPER_SYSTEM_CMD_H #include -#include - +#include #include #include +#include #include @@ -38,26 +38,55 @@ namespace snapper using std::vector; - class SystemCmd : private boost::noncopyable + class SystemCmd final : private boost::noncopyable { public: - SystemCmd(const string& Command_Cv, bool log_output = true); - virtual ~SystemCmd(); + /** + * Class holding command with arguments. + */ + class Args + { + public: + + Args(std::initializer_list init) + : values(init) {} + + const vector& get_values() const { return values; } + + Args& operator<<(const char* arg) { values.push_back(arg); return *this; } + Args& operator<<(const string& arg) { values.push_back(arg); return *this; } + + Args& operator<<(const vector& args) + { values.insert(values.end(), args.begin(), args.end()); return *this; } + + private: + + vector values; + + }; + + + SystemCmd(const Args& args, bool log_output = true); + + ~SystemCmd(); private: enum OutputStream { IDX_STDOUT, IDX_STDERR }; - int execute(const string& Command_Cv); - public: const vector& get_stdout() const { return Lines_aC[IDX_STDOUT]; } const vector& get_stderr() const { return Lines_aC[IDX_STDERR]; } - string cmd() const { return lastCmd; } + /** + * Command as a simple string (without quoting of args - so only for display and + * logging). + */ + string cmd() const; + int retcode() const { return Ret_i; } private: @@ -76,7 +105,7 @@ namespace snapper void invalidate(); void closeOpenFds() const; - int doExecute(const string& Cmd_Cv); + void execute(); bool doWait(int& Ret_ir); void checkOutput(); void getUntilEOF(FILE* File_Cr, vector& Lines_Cr, bool& NewLineSeen_br, @@ -88,26 +117,56 @@ namespace snapper void logOutput() const; + /** - * Constructs the environment for the child process. + * Class to tempararily hold copies for execle() and execvpe(). + */ + class TmpForExec + { + public: + + TmpForExec(const vector& values) : values(values) {} + ~TmpForExec(); + + char* const * get() const { return &values[0]; } + + private: + + vector values; + + }; + + + /** + * Constructs the args for the child process. * * Must not be called after exec since allocating the memory * for the vector is not allowed then (in a multithreaded * program), see fork(2) and signal-safety(7). So simply call * it right before fork. */ - vector make_env() const; + TmpForExec make_args() const; + + /** + * Constructs the environment for the child process. + * + * Same not as for make_args(). + */ + TmpForExec make_env() const; + + + const Args args; + const bool log_output; FILE* File_aC[2]; vector Lines_aC[2]; bool NewLineSeen_ab[2]; - bool log_output; - string lastCmd; - int Ret_i; - int Pid_i; + int Ret_i = 0; + int Pid_i = 0; struct pollfd pfds[2]; static const unsigned line_limit = 50; + }; diff --git a/snapper/Systemctl.cc b/snapper/Systemctl.cc index 982e1b4d..9572904a 100644 --- a/snapper/Systemctl.cc +++ b/snapper/Systemctl.cc @@ -33,8 +33,12 @@ namespace snapper // When run in a chroot system the enable command works but the start command // fails (which is likely what we want). - SystemCmd cmd(SYSTEMCTL_BIN " " + string(enable ? "enable " : "disable ") + - string(now ? "--now " : "") + name); + SystemCmd::Args cmd_args = { SYSTEMCTL_BIN, enable ? "enable" : "disable" }; + if (now) + cmd_args << "--now"; + cmd_args << name; + + SystemCmd cmd(cmd_args); }