From 825c519fe6d26baf10dfb20bb78787f822a8b7bb Mon Sep 17 00:00:00 2001 From: Philipp Gesang Date: Mon, 13 Aug 2018 17:25:58 +0200 Subject: [PATCH] pass pipestream flags as bitset Avoid cluttering the API with long lists of booleans by using symbolic names instead. --- src/pipestream.cpp | 148 ++++++++++++++++++++++++++++++---------------- src/pipestream.hxx | 47 ++++++++------- test/test_pipestream.cpp | 41 +++++++++---- 3 files changed, 150 insertions(+), 86 deletions(-) diff --git a/src/pipestream.cpp b/src/pipestream.cpp index fcd8161..d3f1d3b 100644 --- a/src/pipestream.cpp +++ b/src/pipestream.cpp @@ -45,15 +45,12 @@ on this file might be covered by the GNU General Public License. /** @brief runs command and returns it's output as string * @param command the full command with all parameters * @param rescode struct containing the return code, if the program exited normally and so on - * @param out Whether to collect \c stdout. - * @param err Whether to collect \c stderr; combines with \c out. - * @param path Wether to look up the executable in \c $PATH. + * @param flags runtime control flags (stdio streams, environment, path lookup). * @returns the output (stdout) of the called program */ template std::string capture_exec(CmdT command, ExecResult &rescode, - const bool out, const bool err, - const bool path, const bool env) + const int flags) { std::string output; @@ -67,7 +64,7 @@ std::string capture_exec(CmdT command, ExecResult &rescode, try { { - inpipestream ips(command, out, err, path, env); + inpipestream ips(command, flags); ips.store_exit_status(&exit_set, &exit_status_waitpid); @@ -115,7 +112,7 @@ std::string capture_exec(CmdT command, ExecResult &rescode, * passed. */ std::string capture_exec (const std::string &command, ExecResult &res) -{ return capture_exec(command, res, true, false, false, false); } +{ return capture_exec(command, res, capture_flag::dflt); } /** @brief Instantiation of \c capture_exec for argument lists. The * pipestream used to run the command will not shell out. @@ -127,16 +124,14 @@ std::string capture_exec (const std::string &command, ExecResult &res) * is properly \c NULL terminated. * @param res (Out parameter) Store information about the termination * state in this struct. - * @param out Whether to collect \c stdout. - * @param err Whether to collect \c stderr; combines with \c out. - * @param path Wether to look up the executable in \c $PATH. + * @param flags Runtime control flags (stdio streams, environment, path + * lookup). * * @returns Captured output, combined into one string. */ std::string capture_exec (const char *const *command, ExecResult &res, - const bool out, const bool err, const bool path, - const bool env) -{ return capture_exec(command, res, out, err, path, env); } + const int flags) +{ return capture_exec(command, res, flags); } /** @brief Instantiation of \c capture_exec for argument lists. The * pipestream used to run the command will not shell out. @@ -147,19 +142,14 @@ std::string capture_exec (const char *const *command, ExecResult &res, * assumed to be present at index 0. * @param res (Out parameter) Store information about the termination * state in this struct. - * @param out Whether to collect \c stdout. - * @param err Whether to collect \c stderr; combines with \c out. - * @param path Wether to look up the executable in \c $PATH. + * @param flags Runtime control flags (stdio streams, environment, path + * lookup). * * @returns Captured output, combined into one string. */ std::string capture_exec (const std::vector &command, ExecResult &res, - const bool out, const bool err, const bool path, - const bool env) -{ - return capture_exec &> - (command, res, out, err, path, env); -} + const int flags) +{ return capture_exec &> (command, res, flags); } #define PIPE_CTOR_FAIL(where) \ do { \ @@ -241,30 +231,56 @@ redirect_devnull (const int fd, int &save_errno) /** @brief Helper aggregating common code for the shell-free ctors. * * @param argv Argument list prepared for \c execve(2). - * @param out Whether to capture \c stdout. - * @param err Whether to capture \c stderr. + * @param flags Control the runtime behavior wrt. stdio streams, \c + * *envp, and path search. One of \c collect_out or + * \c collect_err is mandatory. All other flags are + * optional. Pipebuf creation with fail with \c EINVAL + * if that constraint is violated. * * @returns A \c FILE* handle for streaming if successful, \c NULL * otherwise. + * + * Error handling strategy: + * + * - receive all errors from child as ints through a cloexec pipe; + * - in the child, write error conditions always to pipe first, + * then try to emit a more verbose log message; + * - in the parent, throw on error indicating the child errno. + * + * Note that the error-pipe approach is robust due to guarantees by both + * standard (POSIX) and implementation (Linux) of pipes: The read(2) from + * the error channel will block until the pipe is either closed or written to; + * hence no need to check for EAGAIN. Those writes are guaranteed to be atomic + * because sizeof(errno) is less than PIPE_BUF; hence we can disregard EINTR. A + * pipe whose write end (i.e. in the child) has been closed (by the kernel + * because execve(2) was successful) will always indicate EOF by returning + * zero, hence we know precisely whether everything went well or not. Cf. + * pipe(7), sections “I/O on pipes and FIFOs” and “PIPE_BUF”, as well as + * Kerrisk (2010), section 44.10, p. 917f. */ std::pair inpipebuf::init_without_shell (const char *const *argv, - const bool out, - const bool err, - const bool path, - const bool env) const + const int flags) const { FILE *pipeobj = NULL; int pipefd [2]; /* for reading output from the child */ int errfd [2]; /* for determining a successful exec() */ sigset_t oldmask, newmask; - char *const *envp = env ? environ : NULL; + char *const *envp = flags & capture_flag::env_passthru ? environ : NULL; - if (!out && !err) { + if (!(flags & capture_flag::collect_any)) + { errno = EINVAL; PIPE_CTOR_FAIL("ctor"); } + /* + * The error pipe must be openend with *O_CLOEXEC* set. We also open + * the data pipe with close-on-exec and remove that bit only in the child. + * The rationale is preventing the read fd from passed on if the parent + * later re-forks another child: we intend it to be read from this (master) + * process alone. + */ errno = 0; if ( ::pipe2 (pipefd, O_CLOEXEC) == -1 || ::pipe2 (errfd , O_CLOEXEC) == -1) { @@ -283,43 +299,77 @@ inpipebuf::init_without_shell (const char *const *argv, break; } case 0: { + /* + * Close read ends of error and data channels: the child is assumed + * to write exclusively. + */ close (pipefd [0]); close (errfd [0]); + /* + * Remove cloexec bit from the write end of the pipe (this is the + * only flag with F_SETFD). + */ fcntl (pipefd [1], F_SETFD, 0); int save_errno = 0; - if (!out) { - if (!redirect_devnull (STDOUT_FILENO, save_errno)) { + + /* + * Assign /dev/null if asked to close one of the streams, else + * dup() it onto the pipe. + */ + if (!(flags & capture_flag::collect_out)) + { + if (!redirect_devnull (STDOUT_FILENO, save_errno)) + { (void)write (errfd [1], (char *)&save_errno, sizeof(save_errno)); exit (EXIT_FAILURE); } - } else if (dup2 (pipefd[1], STDOUT_FILENO) == -1) { + } + else if (dup2 (pipefd[1], STDOUT_FILENO) == -1) + { (void)write (errfd [1], (char *)&save_errno, sizeof(save_errno)); exit (EXIT_FAILURE); } - if (!err) { - if (!redirect_devnull (STDERR_FILENO, save_errno)) { + if (!(flags & capture_flag::collect_err)) + { + if (!redirect_devnull (STDERR_FILENO, save_errno)) + { (void)write (errfd [1], (char *)&save_errno, sizeof(save_errno)); exit (EXIT_FAILURE); } - } else if (dup2 (pipefd[1], STDERR_FILENO) == -1) { + } + else if (dup2 (pipefd[1], STDERR_FILENO) == -1) + { (void)write (errfd [1], (char *)&save_errno, sizeof(save_errno)); exit (EXIT_FAILURE); } + /* + * Close the write end of the pipe now that we have dup()’ed it + * onto the stdio fds. The parent will now receive EOF on the pipe + * when these fds are both closed. + */ close (pipefd [1]); + /* + * Stop blocking signals so the child starts out with a sane + * environment. + */ sigprocmask (SIG_SETMASK, &oldmask, NULL); errno = 0; - if (path) { + if (flags & capture_flag::search_path) { execvpe (argv [0], const_cast (argv), envp); } else { execve (argv [0], const_cast (argv), envp); } + /* + * At this point, the call to execv[p]e() failed. Thus the error + * pipe is still opened and we forward the errno through it. + */ (void)write (errfd [1], (char *)&errno, sizeof(errno)); exit (EXIT_FAILURE); break; @@ -329,6 +379,10 @@ inpipebuf::init_without_shell (const char *const *argv, } } + /* + * The parent is assumed to only consume data from either pipe, never + * write. + */ close (pipefd [1]); close (errfd [1]); @@ -382,10 +436,7 @@ inpipebuf::init_without_shell (const char *const *argv, } inpipebuf::inpipebuf(const char *const *command, - const bool out, - const bool err, - const bool path, - const bool env) + const int flags) : pipe (NULL) /* brr: shadowing global ident */ , pid (-1) , status_set (NULL) @@ -395,8 +446,7 @@ inpipebuf::inpipebuf(const char *const *command, PIPE_CTOR_FAIL("command"); } - std::pair tmp = - this->init_without_shell (command, out, err, path, env); + std::pair tmp = this->init_without_shell (command, flags); this->pid = tmp.first; /* no std::tie :/ */ this->pipe = tmp.second; @@ -404,10 +454,7 @@ inpipebuf::inpipebuf(const char *const *command, } inpipebuf::inpipebuf(const std::vector &command, - const bool out, - const bool err, - const bool path, - const bool env) + const int flags) : pipe (NULL) /* brr: shadowing global ident */ , pid (-1) , status_set (NULL) @@ -423,7 +470,7 @@ inpipebuf::inpipebuf(const std::vector &command, } std::pair tmp = - this->init_without_shell (argv.get (), out, err, path, env); + this->init_without_shell (argv.get (), flags); this->pid = tmp.first; this->pipe = tmp.second; @@ -431,10 +478,7 @@ inpipebuf::inpipebuf(const std::vector &command, } inpipebuf::inpipebuf(const std::string& command, - const bool _ignored_out, - const bool _ignored_err, - const bool _ignored_path, - const bool _ignored_env) + const int _ignored_flags) : pid (-1) , status_set (NULL) , exit_status (NULL) diff --git a/src/pipestream.hxx b/src/pipestream.hxx index 8668df3..0aed8a4 100644 --- a/src/pipestream.hxx +++ b/src/pipestream.hxx @@ -68,13 +68,24 @@ struct ExecResult }; typedef struct ExecResult ExecResult; +namespace capture_flag { + + static const int none = 0; + static const int collect_out = 1 << 0; + static const int collect_err = 1 << 1; + static const int search_path = 1 << 2; + static const int env_passthru = 1 << 3; + + static const int dflt = collect_out; + static const int collect_any = collect_out | collect_err; + +} /* [namespace capture_flag] */ + std::string capture_exec(const std::string& command, ExecResult &rescode); std::string capture_exec(const char *const *command, ExecResult &rescode, - const bool out=true, const bool err=false, - const bool path=false, const bool env=false); + const int flags=capture_flag::dflt); std::string capture_exec(const std::vector& command, ExecResult &rescode, - const bool out=true, const bool err=false, - const bool path=false, const bool env=false); + const int flags=capture_flag::dflt); inline std::string capture_exec (const std::string &command) { @@ -116,12 +127,9 @@ protected: int *exit_status; public: - inpipebuf(const std::string& command, - const bool out, const bool err, const bool path, const bool env); - inpipebuf(const char *const *command, - const bool out, const bool err, const bool path, const bool env); - inpipebuf(const std::vector &command, - const bool out, const bool err, const bool path, const bool env); + inpipebuf(const std::string& command, const int flags); + inpipebuf(const char *const *command, const int flags); + inpipebuf(const std::vector &command, const int flags); ~inpipebuf(); @@ -132,9 +140,7 @@ protected: private: std::pair - init_without_shell (const char *const *argv, - const bool out, const bool err, - const bool path, const bool env) const; + init_without_shell (const char *const *argv, const int flags) const; }; /** @brief stream around inpipebuf -- see comment there */ @@ -145,21 +151,18 @@ protected: public: inpipestream(const std::string& command, - const bool out=true, const bool err=false, - const bool path=false, const bool env=false) - : std::istream(&buf), buf(command, out, err, path, env) + const int flags=capture_flag::dflt) + : std::istream(&buf), buf(command, flags) {} inpipestream(const char *const command[], - const bool out=true, const bool err=false, - const bool path=false, const bool env=false) - : std::istream(&buf), buf(command, out, err, path, env) + const int flags=capture_flag::dflt) + : std::istream(&buf), buf(command, flags) {} inpipestream(const std::vector &command, - const bool out=true, const bool err=false, - const bool path=false, const bool env=false) - : std::istream(&buf), buf(command, out, err, path, env) + const int flags=capture_flag::dflt) + : std::istream(&buf), buf(command, flags) {} void store_exit_status(bool *_status_set, int *_exit_status) diff --git a/test/test_pipestream.cpp b/test/test_pipestream.cpp index 8924861..743f065 100644 --- a/test/test_pipestream.cpp +++ b/test/test_pipestream.cpp @@ -167,7 +167,8 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) assert (access(bad_command [0], X_OK) != 0); ExecResult exres = ExecResult (); - const std::string result = capture_exec (bad_command, exres, false, true); + const std::string result = capture_exec (bad_command, exres, + capture_flag::collect_err); BOOST_CHECK(!exres.normal_exit); /* failed to exec() */ BOOST_CHECK(!exres.terminated_by_signal); @@ -188,7 +189,7 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) { ExecResult exres = ExecResult (); const std::string result = - capture_exec (false_argv_abs, exres, true, false, false); + capture_exec (false_argv_abs, exres, capture_flag::collect_out); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_FAILURE); @@ -213,7 +214,8 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) { ExecResult exres = ExecResult (); const std::string result = - capture_exec (true_argv_rel, exres, true, false, true); + capture_exec (true_argv_rel, exres, + capture_flag::collect_out | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -225,7 +227,7 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) { ExecResult exres = ExecResult (); const std::string result = - capture_exec (true_argv_rel, exres, true, false, false); + capture_exec (true_argv_rel, exres, capture_flag::collect_out); BOOST_CHECK(!exres.normal_exit); /* failed to exec() */ /* no return code check since we couldn't exit */ @@ -240,7 +242,8 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) { ExecResult exres = ExecResult (); const std::string result = - capture_exec (true_argv_abs, exres, true, false, true); + capture_exec (true_argv_abs, exres, + capture_flag::collect_out | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -252,7 +255,8 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) { ExecResult exres = ExecResult (); const std::string result = - capture_exec (false_argv_rel, exres, true, false, true); + capture_exec (false_argv_rel, exres, + capture_flag::collect_out | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); /* no return code check since we couldn't exit */ @@ -280,7 +284,9 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) ExecResult exres = ExecResult (); const std::string text = "The significant owl hoots in the night."; const std::vector argv = mk_echo_argv (text); - const std::string result = capture_exec (argv, exres, true, false, true); + const std::string result = capture_exec (argv, exres, + capture_flag::collect_out + | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -292,7 +298,9 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) ExecResult exres = ExecResult (); const std::string text = "Yet many grey lords go sadly to the masterless men."; const std::vector argv = mk_echo_argv (text, false); - const std::string result = capture_exec (argv, exres, true, false, true); + const std::string result = capture_exec (argv, exres, + capture_flag::collect_out + | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -320,7 +328,9 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) ExecResult exres = ExecResult (); const std::string text = "Hooray, hooray for the spinster’s sister’s daughter."; const std::vector argv = mk_errcho_argv (text); - const std::string result = capture_exec (argv, exres, false, true, true); + const std::string result = capture_exec (argv, exres, + capture_flag::collect_err + | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -332,7 +342,9 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) ExecResult exres = ExecResult (); const std::string text = "To the axeman, all supplicants are the same height."; const std::vector argv = mk_errcho_argv (text); - const std::string result = capture_exec (argv, exres, true, false, true); + const std::string result = capture_exec (argv, exres, + capture_flag::collect_out + | capture_flag::search_path); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -357,7 +369,10 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) environ = i2n_extra_environ; const std::string result = - capture_exec (env_argv_abs, exres, true, true, false, true); + capture_exec (env_argv_abs, exres, + capture_flag::collect_out + | capture_flag::collect_err + | capture_flag::env_passthru); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); @@ -370,7 +385,9 @@ BOOST_FIXTURE_TEST_SUITE(pipestream, Test_Pipestream_Fixture) environ = i2n_extra_environ; const std::string result = - capture_exec (env_argv_abs, exres, true, true, false, false); + capture_exec (env_argv_abs, exres, + capture_flag::collect_out + | capture_flag::collect_err); BOOST_CHECK(exres.normal_exit); BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS); -- 1.7.1