pass pipestream flags as bitset
authorPhilipp Gesang <philipp.gesang@intra2net.com>
Mon, 13 Aug 2018 15:25:58 +0000 (17:25 +0200)
committerPhilipp Gesang <philipp.gesang@intra2net.com>
Tue, 14 Aug 2018 14:53:34 +0000 (16:53 +0200)
Avoid cluttering the API with long lists of booleans by using
symbolic names instead.

src/pipestream.cpp
src/pipestream.hxx
test/test_pipestream.cpp

index fcd8161..d3f1d3b 100644 (file)
@@ -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 <typename CmdT>
 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<const std::string &>(command, res, true, false, false, false); }
+{ return capture_exec<const std::string &>(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<const char *const *>(command, res, out, err, path, env); }
+                          const int flags)
+{ return capture_exec<const char *const *>(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<std::string> &command, ExecResult &res,
-                          const bool out, const bool err, const bool path,
-                          const bool env)
-{
-    return capture_exec<const std::vector<std::string> &>
-        (command, res, out, err, path, env);
-}
+                          const int flags)
+{ return capture_exec<const std::vector<std::string> &> (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 <pid_t, FILE *>
 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 <char *const *>(argv), envp);
             } else {
                 execve (argv [0], const_cast <char *const *>(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 <pid_t, FILE *> tmp =
-        this->init_without_shell (command, out, err, path, env);
+    std::pair <pid_t, FILE *> 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<std::string> &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<std::string> &command,
     }
 
     std::pair <pid_t, FILE *> 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<std::string> &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)
index 8668df3..0aed8a4 100644 (file)
@@ -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<std::string>& 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<std::string> &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<std::string> &command, const int flags);
 
     ~inpipebuf();
 
@@ -132,9 +140,7 @@ protected:
 
 private:
     std::pair <pid_t, FILE *>
-    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<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)
     {}
 
     void store_exit_status(bool *_status_set, int *_exit_status)
index 8924861..743f065 100644 (file)
@@ -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<std::string> 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<std::string> 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<std::string> 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<std::string> 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);