From 53b4f58c78e21cbc442891c2ce2a2b99a32e47bc Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 14 Dec 2017 14:24:38 +0200 Subject: Add process::pipe struct, extend process API --- libbutl/curl.cxx | 40 ++++++++++++----- libbutl/curl.mxx | 8 ++-- libbutl/openssl.cxx | 38 ++++++++++------ libbutl/openssl.mxx | 12 ++--- libbutl/process-run.cxx | 6 +-- libbutl/process-run.txx | 104 ++++++++++++++++++++++++++++++++++++------- libbutl/process.cxx | 65 ++++++++++++++++++++------- libbutl/process.ixx | 24 ++++++++++ libbutl/process.mxx | 39 ++++++++++++---- libbutl/sendmail.ixx | 2 +- tests/process-run/driver.cxx | 7 +++ 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 + #ifndef __cpp_lib_modules #include @@ -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 std::enable_if::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 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 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 std::enable_if::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 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 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 (in))); - int out_i (process_stdout (std::forward (out))); - int err_i (process_stderr (std::forward (err))); + process::pipe in_i (process_stdin (std::forward (in))); + process::pipe out_i (process_stdout (std::forward (out))); + process::pipe err_i (process_stderr (std::forward (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 (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")); -- cgit v1.1