Add process::pipe struct, extend process API

11 files changed, 265 insertions, 80 deletions

diff --git a/libbutl/curl.cxx b/libbutl/curl.cxx
index 5ff81ab..ee61d6d 100644
--- a/libbutl/curl.cxx
+++ b/libbutl/curl.cxx
@@ -8,6 +8,8 @@
 
 // C includes.
 
+#include <cassert>
+
 #ifndef __cpp_lib_modules
 #include <string>
 
@@ -40,7 +42,7 @@ using namespace std;
 
 namespace butl
 {
-  int curl::
+  process::pipe curl::
   map_in (nullfd_t, method_proto mp, io_data& d)
   {
     switch (mp)
@@ -53,16 +55,18 @@ namespace butl
     case http_get:
       {
         d.pipe.in = fdnull (); // /dev/null
-        return d.pipe.in.get ();
+        return pipe (d.pipe);
       }
     }
 
-    return -1;
+    assert (false); // Can't be here.
+    return pipe ();
   }
 
-  int curl::
+  process::pipe curl::
   map_in (const path& f, method_proto mp, io_data& d)
   {
+    pipe r;
     switch (mp)
     {
     case ftp_put:
@@ -84,12 +88,17 @@ namespace butl
         if (f.string () == "-")
         {
           d.pipe = fdopen_pipe (fdopen_mode::binary);
+          r = pipe (d.pipe);
+
           out.open (move (d.pipe.out));
         }
         else
+        {
           d.pipe.in = fdnull (); // /dev/null
+          r = pipe (d.pipe);
+        }
 
-        return d.pipe.in.get ();
+        return r;
       }
     case ftp_get:
     case http_get:
@@ -98,10 +107,11 @@ namespace butl
       }
     }
 
-    return -1;
+    assert (false); // Can't be here.
+    return r;
   }
 
-  int curl::
+  process::pipe curl::
   map_out (nullfd_t, method_proto mp, io_data& d)
   {
     switch (mp)
@@ -113,16 +123,18 @@ namespace butl
     case http_post: // May or may not produce output.
       {
         d.pipe.out = fdnull ();
-        return d.pipe.out.get (); // /dev/null
+        return pipe (d.pipe); // /dev/null
       }
     }
 
-    return -1;
+    assert (false); // Can't be here.
+    return pipe ();
   }
 
-  int curl::
+  process::pipe curl::
   map_out (const path& f, method_proto mp, io_data& d)
   {
+    pipe r;
     switch (mp)
     {
     case ftp_get:
@@ -134,6 +146,8 @@ namespace butl
           // Note: no need for any options, curl writes to stdout by default.
           //
           d.pipe = fdopen_pipe (fdopen_mode::binary);
+          r = pipe (d.pipe);
+
           in.open (move (d.pipe.in));
         }
         else
@@ -141,9 +155,10 @@ namespace butl
           d.options.push_back ("-o");
           d.options.push_back (f.string ().c_str ());
           d.pipe.out = fdnull (); // /dev/null
+          r = pipe (d.pipe);
         }
 
-        return d.pipe.out.get ();
+        return r;
       }
     case ftp_put:
       {
@@ -151,7 +166,8 @@ namespace butl
       }
     }
 
-    return -1;
+    assert (false); // Can't be here.
+    return r;
   }
 
   curl::method_proto curl::
diff --git a/libbutl/curl.mxx b/libbutl/curl.mxx
index 57d6e56..7beb844 100644
--- a/libbutl/curl.mxx
+++ b/libbutl/curl.mxx
@@ -169,20 +169,20 @@ LIBBUTL_MODEXPORT namespace butl
       std::string storage;
     };
 
-    int
+    pipe
     map_in (nullfd_t, method_proto, io_data&);
 
-    int
+    pipe
     map_in (const path&, method_proto, io_data&);
 
     template <typename I>
     typename std::enable_if<is_other<I>::value, I>::type
     map_in (I&&, method_proto, io_data&);
 
-    int
+    pipe
     map_out (nullfd_t, method_proto, io_data&);
 
-    int
+    pipe
     map_out (const path&, method_proto, io_data&);
 
     template <typename O>
diff --git a/libbutl/openssl.cxx b/libbutl/openssl.cxx
index 89cf8ed..48b0c1d 100644
--- a/libbutl/openssl.cxx
+++ b/libbutl/openssl.cxx
@@ -36,21 +36,24 @@ using namespace std;
 
 namespace butl
 {
-  int openssl::
+  process::pipe openssl::
   map_in (nullfd_t, io_data& d)
   {
     d.pipe.in = fdnull (); // /dev/null
-    return d.pipe.in.get ();
+    return pipe (d.pipe);
   }
 
-  int openssl::
+  process::pipe openssl::
   map_in (const path& f, io_data& d)
   {
+    pipe r;
     if (f.string () == "-")
     {
       // Note: no need for any options, openssl reads from stdin by default.
       //
       d.pipe = fdopen_pipe (fdopen_mode::binary);
+      r = pipe (d.pipe);
+
       out.open (move (d.pipe.out));
     }
     else
@@ -58,12 +61,13 @@ namespace butl
       d.options.push_back ("-in");
       d.options.push_back (f.string ().c_str ());
       d.pipe.in = fdnull (); // /dev/null
+      r = pipe (d.pipe);
     }
 
-    return d.pipe.in.get ();
+    return r;
   }
 
-  int openssl::
+  process::pipe openssl::
   map_in (fdstream_mode m, io_data& d)
   {
     assert (m == fdstream_mode::text || m == fdstream_mode::binary);
@@ -73,26 +77,30 @@ namespace butl
     d.pipe = fdopen_pipe (m == fdstream_mode::binary
                           ? fdopen_mode::binary
                           : fdopen_mode::none);
-
+    pipe r (d.pipe);
+    
     out.open (move (d.pipe.out));
-    return d.pipe.in.get ();
+    return r;
   }
 
-  int openssl::
+  process::pipe openssl::
   map_out (nullfd_t, io_data& d)
   {
     d.pipe.out = fdnull ();
-    return d.pipe.out.get (); // /dev/null
+    return pipe (d.pipe); // /dev/null
   }
 
-  int openssl::
+  process::pipe openssl::
   map_out (const path& f, io_data& d)
   {
+    pipe r;
     if (f.string () == "-")
     {
       // Note: no need for any options, openssl writes to stdout by default.
       //
       d.pipe = fdopen_pipe (fdopen_mode::binary);
+      r = pipe (d.pipe);
+
       in.open (move (d.pipe.in), fdstream_mode::skip);
     }
     else
@@ -100,12 +108,13 @@ namespace butl
       d.options.push_back ("-out");
       d.options.push_back (f.string ().c_str ());
       d.pipe.out = fdnull (); // /dev/null
+      r = pipe (d.pipe);
     }
 
-    return d.pipe.out.get ();
+    return r;
   }
 
-  int openssl::
+  process::pipe openssl::
   map_out (fdstream_mode m, io_data& d)
   {
     assert (m == fdstream_mode::text || m == fdstream_mode::binary);
@@ -115,8 +124,9 @@ namespace butl
     d.pipe = fdopen_pipe (m == fdstream_mode::binary
                           ? fdopen_mode::binary
                           : fdopen_mode::none);
-
+    pipe r (d.pipe);
+    
     in.open (move (d.pipe.in), fdstream_mode::skip);
-    return d.pipe.out.get ();
+    return r;
   }
 }
diff --git a/libbutl/openssl.mxx b/libbutl/openssl.mxx
index 42afc5e..a465aad 100644
--- a/libbutl/openssl.mxx
+++ b/libbutl/openssl.mxx
@@ -152,26 +152,26 @@ LIBBUTL_MODEXPORT namespace butl
       small_vector<const char*, 2> options;
     };
 
-    int
+    pipe
     map_in (nullfd_t, io_data&);
 
-    int
+    pipe
     map_in (const path&, io_data&);
 
-    int
+    pipe
     map_in (fdstream_mode, io_data&);
 
     template <typename I>
     typename std::enable_if<is_other<I>::value, I>::type
     map_in (I&&, io_data&);
 
-    int
+    pipe
     map_out (nullfd_t, io_data&);
 
-    int
+    pipe
     map_out (const path&, io_data&);
 
-    int
+    pipe
     map_out (fdstream_mode, io_data&);
 
     template <typename O>
diff --git a/libbutl/process-run.cxx b/libbutl/process-run.cxx
index fd8abe3..446eec0 100644
--- a/libbutl/process-run.cxx
+++ b/libbutl/process-run.cxx
@@ -41,9 +41,9 @@ namespace butl
                  const process_path& pp,
                  const char* cmd[],
                  const char* const* envvars,
-                 int in,
-                 int out,
-                 int err)
+                 process::pipe in,
+                 process::pipe out,
+                 process::pipe err)
   {
     try
     {
diff --git a/libbutl/process-run.txx b/libbutl/process-run.txx
index d8d4cb7..9e4ccfc 100644
--- a/libbutl/process-run.txx
+++ b/libbutl/process-run.txx
@@ -20,27 +20,101 @@ LIBBUTL_MODEXPORT namespace butl //@@ MOD Clang needs this for some reason.
     }
   }
 
-  inline int process_stdin  (int v) {assert (v >= 0); return v;}
-  inline int process_stdout (int v) {assert (v >= 0); return v;}
-  inline int process_stderr (int v) {assert (v >= 0); return v;}
+  inline process::pipe
+  process_stdin  (int v)
+  {
+    assert (v >= 0);
+    return process::pipe (v, -1);
+  }
+
+  inline process::pipe
+  process_stdout (int v)
+  {
+    assert (v >= 0);
+    return process::pipe (-1, v);
+  }
+
+  inline process::pipe
+  process_stderr (int v)
+  {
+    assert (v >= 0);
+    return process::pipe (-1, v);
+  }
+
+  inline process::pipe
+  process_stdin (const auto_fd& v)
+  {
+    assert (v.get () >= 0);
+    return process::pipe (v.get (), -1);
+  }
+
+  inline process::pipe
+  process_stdout (const auto_fd& v)
+  {
+    assert (v.get () >= 0);
+    return process::pipe (-1, v.get ());
+  }
+
+  inline process::pipe
+  process_stderr (const auto_fd& v)
+  {
+    assert (v.get () >= 0);
+    return process::pipe (-1, v.get ());
+  }
+
+  inline process::pipe
+  process_stdin (const fdpipe& v)
+  {
+    assert (v.in.get () >= 0 && v.out.get () >= 0);
+    return process::pipe (v);
+  }
+
+  inline process::pipe
+  process_stdout (const fdpipe& v)
+  {
+    assert (v.in.get () >= 0 && v.out.get () >= 0);
+    return process::pipe (v);
+  }
+
+  inline process::pipe
+  process_stderr (const fdpipe& v)
+  {
+    assert (v.in.get () >= 0 && v.out.get () >= 0);
+    return process::pipe (v);
+  }
 
-  inline int
-  process_stdin (const auto_fd& v) {assert (v.get () >= 0); return v.get ();}
+  // Not necessarily a real pipe, so only a single end is constrained to be a
+  // valid file descriptor.
+  //
+  inline process::pipe
+  process_stdin (const process::pipe& v)
+  {
+    assert (v.in >= 0);
+    return v;
+  }
 
-  inline int
-  process_stdout (const auto_fd& v) {assert (v.get () >= 0); return v.get ();}
+  inline process::pipe
+  process_stdout (const process::pipe& v)
+  {
+    assert (v.out >= 0);
+    return v;
+  }
 
-  inline int
-  process_stderr (const auto_fd& v) {assert (v.get () >= 0); return v.get ();}
+  inline process::pipe
+  process_stderr (const process::pipe& v)
+  {
+    assert (v.out >= 0);
+    return v;
+  }
 
   LIBBUTL_SYMEXPORT process
   process_start (const dir_path* cwd,
                  const process_path& pp,
                  const char* cmd[],
                  const char* const* envvars,
-                 int in,
-                 int out,
-                 int err);
+                 process::pipe in,
+                 process::pipe out,
+                 process::pipe err);
 
   template <typename V, typename T>
   inline const char*
@@ -68,9 +142,9 @@ LIBBUTL_MODEXPORT namespace butl //@@ MOD Clang needs this for some reason.
     // Map stdin/stdout/stderr arguments to their integer values, as expected
     // by the process constructor.
     //
-    int in_i  (process_stdin  (std::forward<I> (in)));
-    int out_i (process_stdout (std::forward<O> (out)));
-    int err_i (process_stderr (std::forward<E> (err)));
+    process::pipe in_i  (process_stdin  (std::forward<I> (in)));
+    process::pipe out_i (process_stdout (std::forward<O> (out)));
+    process::pipe err_i (process_stderr (std::forward<E> (err)));
 
     // Construct the command line array.
     //
diff --git a/libbutl/process.cxx b/libbutl/process.cxx
index b9f3b2f..4a6d5f5 100644
--- a/libbutl/process.cxx
+++ b/libbutl/process.cxx
@@ -170,17 +170,6 @@ namespace butl
     } while (*p != nullptr);
   }
 
-  process::
-  process (const process_path& pp, const char* args[],
-           process& in, int out, int err,
-           const char* cwd,
-           const char* const* envvars)
-      : process (pp, args, in.in_ofd.get (), out, err, cwd, envvars)
-  {
-    assert (in.in_ofd.get () != -1); // Should be a pipe.
-    in.in_ofd.reset (); // Close it on our side.
-  }
-
 #ifndef _WIN32
 
   static process_path
@@ -303,10 +292,14 @@ namespace butl
 
   process::
   process (const process_path& pp, const char* args[],
-           int in, int out, int err,
+           pipe pin, pipe pout, pipe perr,
            const char* cwd,
            const char* const* envvars)
   {
+    int in  (pin.in);
+    int out (pout.out);
+    int err (perr.out);
+
     fdpipe out_fd;
     fdpipe in_ofd;
     fdpipe in_efd;
@@ -1013,10 +1006,14 @@ namespace butl
 
   process::
   process (const process_path& pp, const char* args[],
-           int in, int out, int err,
+           pipe pin, pipe pout, pipe perr,
            const char* cwd,
            const char* const* envvars)
   {
+    int in  (pin.in);
+    int out (pout.out);
+    int err (perr.out);
+
     auto fail = [] (const char* m = nullptr)
     {
       throw process_error (m == nullptr ? last_error_msg () : m);
@@ -1487,6 +1484,31 @@ namespace butl
           // data presence at the reading end of any of these pipes means that
           // DLLs initialization successfully passed.
           //
+          // If the process output stream is redirected to a pipe, check that
+          // we have access to its reading end to probe it for data presence.
+          //
+          // @@ Unfortunately we can't distinguish a pipe that erroneously
+          //    missing the reading end from the pipelined program output
+          //    stream.
+#if 0
+          auto bad_pipe = [&get_osfhandle] (const pipe& p) -> bool
+          {
+            if (p.in != -1  ||  // Pipe provided by the user.
+                p.out == -1 ||  // Pipe created by ctor.
+                p.out == -2 ||  // Null device.
+                fdterm (p.out)) // Terminal.
+              return false;
+
+            // No reading end, so make sure that the file descriptor is a pipe.
+            //
+            return GetFileType (get_osfhandle (p.out, false)) ==
+                   FILE_TYPE_PIPE;
+          };
+
+          if (bad_pipe (pout) || bad_pipe (perr))
+            assert (false);
+#endif
+
           system_clock::time_point st (system_clock::now ());
 
           // Unlock the mutex to let other processes to be spawned while we are
@@ -1496,14 +1518,22 @@ namespace butl
           il.unlock ();
           l.unlock ();
 
-          auto probe_fd = [&get_osfhandle] (int fd) -> bool
+          // Probe for data presence the reading end of the pipe that is
+          // connected to the process standard output/error stream. Note that
+          // the pipe can be created by us or provided by the user.
+          //
+          auto probe_fd = [&get_osfhandle] (int fd, int ufd) -> bool
           {
-            if (fd == -1)
+            // Can't be both created by us and provided by the user.
+            //
+            assert (fd == -1 || ufd == -1);
+
+            if (fd == -1 && ufd == -1)
               return false;
 
             char c;
             DWORD n;
-            HANDLE h (get_osfhandle (fd, false));
+            HANDLE h (get_osfhandle (fd != -1 ? fd : ufd, false));
             return PeekNamedPipe (h, &c, 1, &n, nullptr, nullptr) && n == 1;
           };
 
@@ -1519,7 +1549,8 @@ namespace butl
             twd += wd;
 
             if (r != WAIT_TIMEOUT ||
-                probe_fd (in_ofd.in.get ()) || probe_fd (in_efd.in.get ()))
+                probe_fd (in_ofd.in.get (), pout.in) ||
+                probe_fd (in_efd.in.get (), perr.in))
               break;
           }
 
diff --git a/libbutl/process.ixx b/libbutl/process.ixx
index fb076cf..a0e2de6 100644
--- a/libbutl/process.ixx
+++ b/libbutl/process.ixx
@@ -137,6 +137,19 @@ namespace butl
   }
 
   inline process::
+  process (const process_path& pp, const char* args[],
+           int in, int out, int err,
+           const char* cwd,
+           const char* const* envvars)
+      : process (pp,
+                 args,
+                 pipe (in, -1), pipe (-1, out), pipe (-1, err),
+                 cwd,
+                 envvars)
+  {
+  }
+
+  inline process::
   process (const char* args[],
            int in, int out, int err,
            const char* cwd,
@@ -144,6 +157,17 @@ namespace butl
       : process (path_search (args[0]), args, in, out, err, cwd, envvars) {}
 
   inline process::
+  process (const process_path& pp, const char* args[],
+           process& in, int out, int err,
+           const char* cwd,
+           const char* const* envvars)
+      : process (pp, args, in.in_ofd.get (), out, err, cwd, envvars)
+  {
+    assert (in.in_ofd.get () != -1); // Should be a pipe.
+    in.in_ofd.reset (); // Close it on our side.
+  }
+
+  inline process::
   process (const char* args[],
            process& in, int out, int err,
            const char* cwd,
diff --git a/libbutl/process.mxx b/libbutl/process.mxx
index abf7d35..af6f995 100644
--- a/libbutl/process.mxx
+++ b/libbutl/process.mxx
@@ -255,12 +255,35 @@ LIBBUTL_MODEXPORT namespace butl
     // temporarily change args[0] (see path_search() for details).
     //
     process (const char* [],
-             int = 0, int = 1, int = 2,
+             int in = 0, int out = 1, int err = 2,
              const char* cwd = nullptr,
              const char* const* envvars = nullptr);
 
     process (const process_path&, const char* [],
-             int = 0, int = 1, int = 2,
+             int in = 0, int out = 1, int err = 2,
+             const char* cwd = nullptr,
+             const char* const* envvars = nullptr);
+
+    // If the descriptors are pipes that you have created, then you should use
+    // this constructor instead to communicate this information.
+    //
+    // For generality, if the "other" end of the pipe is -1, then assume this
+    // is not a pipe.
+    //
+    struct pipe
+    {
+      int in  = -1;
+      int out = -1;
+
+      pipe () = default;
+      pipe (int i, int o): in (i), out (o) {}
+
+      explicit
+      pipe (const fdpipe& p): in (p.in.get ()), out (p.out.get ()) {}
+    };
+
+    process (const process_path&, const char* [],
+             pipe in, pipe out, pipe err,
              const char* cwd = nullptr,
              const char* const* envvars = nullptr);
 
@@ -273,12 +296,12 @@ LIBBUTL_MODEXPORT namespace butl
     // lhs.wait ();
     //
     process (const char* [],
-             process&, int = 1, int = 2,
+             process&, int out = 1, int err = 2,
              const char* cwd = nullptr,
              const char* const* envvars = nullptr);
 
     process (const process_path&, const char* [],
-             process&, int = 1, int = 2,
+             process&, int out = 1, int err = 2,
              const char* cwd = nullptr,
              const char* const* envvars = nullptr);
 
@@ -427,10 +450,10 @@ LIBBUTL_MODEXPORT namespace butl
   // exit status.
   //
   // The I/O/E arguments determine the child's stdin/stdout/stderr. They can
-  // be of type int, auto_fd (and, in the future, perhaps also fd_pipe,
-  // string, buffer, etc). For example, the following call will make stdin
-  // read from /dev/null, stdout redirect to stderr, and inherit the parent's
-  // stderr.
+  // be of type int, auto_fd, fd_pipe and process::pipe (and, in the future,
+  // perhaps also string, buffer, etc). For example, the following call will
+  // make stdin read from /dev/null, stdout redirect to stderr, and inherit
+  // the parent's stderr.
   //
   // process_run (fdnull (), 2, 2, ...)
   //
diff --git a/libbutl/sendmail.ixx b/libbutl/sendmail.ixx
index 942b9a8..4fb3dac 100644
--- a/libbutl/sendmail.ixx
+++ b/libbutl/sendmail.ixx
@@ -60,7 +60,7 @@ LIBBUTL_MODEXPORT namespace butl //@@ MOD Clang needs this for some reason.
 
     process& p (*this);
     p = process_start_callback (cmdc,
-                                pipe.in,
+                                pipe,
                                 2, // No output expected so redirect to stderr.
                                 std::forward<E> (err),
                                 "sendmail",
diff --git a/tests/process-run/driver.cxx b/tests/process-run/driver.cxx
index 765bd7d..1c41659 100644
--- a/tests/process-run/driver.cxx
+++ b/tests/process-run/driver.cxx
@@ -99,6 +99,13 @@ main (int argc, const char* argv[])
   assert (run (fdnull (), 2, 2, p, "-c", "-o", "abc"));
   assert (run (fdnull (), 1, 1, p, "-c", "-e", "abc"));
 
+  {
+    fdpipe pipe (fdopen_pipe ());
+    process pr (process_start (pipe, process::pipe (-1, 1), 2, p, "-c", "-i"));
+    pipe.close ();
+    assert (pr.wait ());
+  }
+
   // Argument conversion.
   //
   assert (run (0, 1, 2, p, "-c", "-o", "abc"));