From 96bd762fc047066ccad4f00dba4de95abadfae98 Mon Sep 17 00:00:00 2001 From: Tudor Bosman Date: Sat, 3 Nov 2012 20:03:23 -0700 Subject: [PATCH] ints used as flags (bitwise): so C Summary: Changed communicate() flags from int to a class. Made Options and CommunicateFlags composable with | Simplified API so you don't have to type Subprocess::Options().stdout(Subprocess::PIPE) Test Plan: subprocess_test Reviewed By: chip@fb.com FB internal diff: D620186 --- folly/Subprocess.cpp | 32 +++++---------- folly/Subprocess.h | 76 +++++++++++++++++++++++++++++------ folly/test/SubprocessTest.cpp | 19 ++++----- 3 files changed, 80 insertions(+), 47 deletions(-) diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp index 009c5c0e..9070f4ca 100644 --- a/folly/Subprocess.cpp +++ b/folly/Subprocess.cpp @@ -196,22 +196,8 @@ Subprocess::Subprocess( } Subprocess::~Subprocess() { - if (returnCode_.state() == ProcessReturnCode::RUNNING) { - LOG(ERROR) << "Subprocess destroyed without reaping; killing child."; - try { - kill(); - wait(); - } catch (...) { - LOG(FATAL) << "Killing child failed, terminating: " - << exceptionStr(std::current_exception()); - } - } - try { - closeAll(); - } catch (...) { - LOG(FATAL) << "close failed, terminating: " - << exceptionStr(std::current_exception()); - } + CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING) + << "Subprocess destroyed without reaping child"; } namespace { @@ -531,7 +517,7 @@ bool discardRead(int fd) { } // namespace std::pair Subprocess::communicate( - int flags, + const CommunicateFlags& flags, StringPiece data) { IOBufQueue dataQueue; dataQueue.wrapBuffer(data.data(), data.size()); @@ -554,14 +540,14 @@ std::pair Subprocess::communicate( } std::pair Subprocess::communicateIOBuf( - int flags, + const CommunicateFlags& flags, IOBufQueue data) { std::pair out; - auto readCallback = [&, flags] (int pfd, int cfd) { - if (cfd == 1 && (flags & READ_STDOUT)) { + auto readCallback = [&] (int pfd, int cfd) { + if (cfd == 1 && flags.readStdout_) { return handleRead(pfd, out.first); - } else if (cfd == 2 && (flags & READ_STDERR)) { + } else if (cfd == 2 && flags.readStderr_) { return handleRead(pfd, out.second); } else { // Don't close the file descriptor, the child might not like SIGPIPE, @@ -570,8 +556,8 @@ std::pair Subprocess::communicateIOBuf( } }; - auto writeCallback = [&, flags] (int pfd, int cfd) { - if (cfd == 0 && (flags & WRITE_STDIN)) { + auto writeCallback = [&] (int pfd, int cfd) { + if (cfd == 0 && flags.writeStdin_) { return handleWrite(pfd, data); } else { // If we don't want to write to this fd, just close it. diff --git a/folly/Subprocess.h b/folly/Subprocess.h index 096b1bee..cd083fa2 100644 --- a/folly/Subprocess.h +++ b/folly/Subprocess.h @@ -33,12 +33,12 @@ * to complete, returning the exit status. * * A thread-safe version of popen() (type="r", to read from the child): - * Subprocess proc(cmd, Subprocess::Options().stdout(Subprocess::PIPE)); + * Subprocess proc(cmd, Subprocess::pipeStdout()); * // read from proc.stdout() * proc.wait(); * * A thread-safe version of popen() (type="w", to write from the child): - * Subprocess proc(cmd, Subprocess::Options().stdin(Subprocess::PIPE)); + * Subprocess proc(cmd, Subprocess::pipeStdin()); * // write to proc.stdin() * proc.wait(); * @@ -176,7 +176,7 @@ class Subprocess : private boost::noncopyable { * the close-on-exec flag is set (fcntl FD_CLOEXEC) and inherited * otherwise. */ - class Options { + class Options : private boost::orable { friend class Subprocess; public: Options() : closeOtherFds_(false), usePath_(false) { } @@ -232,6 +232,12 @@ class Subprocess : private boost::noncopyable { * Use the search path ($PATH) when searching for the executable. */ Options& usePath() { usePath_ = true; return *this; } + + /** + * Helpful way to combine Options. + */ + Options& operator|=(const Options& other); + private: typedef boost::container::flat_map FdMap; FdMap fdActions_; @@ -239,6 +245,10 @@ class Subprocess : private boost::noncopyable { bool usePath_; }; + static Options pipeStdin() { return Options().stdin(PIPE); } + static Options pipeStdout() { return Options().stdout(PIPE); } + static Options pipeStderr() { return Options().stderr(PIPE); } + /** * Create a subprocess from the given arguments. argv[0] must be listed. * If not-null, executable must be the actual executable @@ -270,14 +280,14 @@ class Subprocess : private boost::noncopyable { * Append all data, close the stdin (to-child) fd, and read all data, * except that this is done in a safe manner to prevent deadlocking. * - * If WRITE_STDIN is given in flags, the process must have been opened with + * If writeStdin() is given in flags, the process must have been opened with * stdinFd=PIPE. * - * If READ_STDOUT is given in flags, the first returned value will be the + * If readStdout() is given in flags, the first returned value will be the * value read from the child's stdout; the child must have been opened with * stdoutFd=PIPE. * - * If READ_STDERR is given in flags, the second returned value will be the + * If readStderr() is given in flags, the second returned value will be the * value read from the child's stderr; the child must have been opened with * stderrFd=PIPE. * @@ -288,17 +298,38 @@ class Subprocess : private boost::noncopyable { * that it won't try to allocate all data at once). communicate * uses strings for simplicity. */ - enum { - WRITE_STDIN = 1 << 0, - READ_STDOUT = 1 << 1, - READ_STDERR = 1 << 2, + class CommunicateFlags : private boost::orable { + friend class Subprocess; + public: + CommunicateFlags() + : writeStdin_(false), readStdout_(false), readStderr_(false) { } + CommunicateFlags& writeStdin() { writeStdin_ = true; return *this; } + CommunicateFlags& readStdout() { readStdout_ = true; return *this; } + CommunicateFlags& readStderr() { readStderr_ = true; return *this; } + + CommunicateFlags& operator|=(const CommunicateFlags& other); + private: + bool writeStdin_; + bool readStdout_; + bool readStderr_; }; + + static CommunicateFlags writeStdin() { + return CommunicateFlags().writeStdin(); + } + static CommunicateFlags readStdout() { + return CommunicateFlags().readStdout(); + } + static CommunicateFlags readStderr() { + return CommunicateFlags().readStderr(); + } + std::pair communicateIOBuf( - int flags = READ_STDOUT, + const CommunicateFlags& flags = readStdout(), IOBufQueue data = IOBufQueue()); std::pair communicate( - int flags = READ_STDOUT, + const CommunicateFlags& flags = readStdout(), StringPiece data = StringPiece()); /** @@ -452,6 +483,27 @@ class Subprocess : private boost::noncopyable { std::vector pipes_; }; +inline Subprocess::Options& Subprocess::Options::operator|=( + const Subprocess::Options& other) { + if (this == &other) return *this; + // Replace + for (auto& p : other.fdActions_) { + fdActions_[p.first] = p.second; + } + closeOtherFds_ |= other.closeOtherFds_; + usePath_ |= other.usePath_; + return *this; +} + +inline Subprocess::CommunicateFlags& Subprocess::CommunicateFlags::operator|=( + const Subprocess::CommunicateFlags& other) { + if (this == &other) return *this; + writeStdin_ |= other.writeStdin_; + readStdout_ |= other.readStdout_; + readStderr_ |= other.readStderr_; + return *this; +} + } // namespace folly #endif /* FOLLY_SUBPROCESS_H_ */ diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp index 7f6b96dd..d0203cab 100644 --- a/folly/test/SubprocessTest.cpp +++ b/folly/test/SubprocessTest.cpp @@ -55,7 +55,7 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) { } TEST(PopenSubprocessTest, PopenRead) { - Subprocess proc("ls /", Subprocess::Options().stdout(Subprocess::PIPE)); + Subprocess proc("ls /", Subprocess::pipeStdout()); int found = 0; for (auto bline : byLine(proc.stdout())) { StringPiece line(bline); @@ -69,7 +69,7 @@ TEST(PopenSubprocessTest, PopenRead) { TEST(CommunicateSubprocessTest, SimpleRead) { Subprocess proc(std::vector{ "/bin/echo", "-n", "foo", "bar"}, - Subprocess::Options().stdout(Subprocess::PIPE)); + Subprocess::pipeStdout()); auto p = proc.communicate(); EXPECT_EQ("foo bar", p.first); proc.waitChecked(); @@ -84,11 +84,8 @@ TEST(CommunicateSubprocessTest, BigWrite) { data.append(line); } - Subprocess::Options options; - options.stdin(Subprocess::PIPE).stdout(Subprocess::PIPE); - - Subprocess proc("wc -l", options); - auto p = proc.communicate(Subprocess::WRITE_STDIN | Subprocess::READ_STDOUT, + Subprocess proc("wc -l", Subprocess::pipeStdin() | Subprocess::pipeStdout()); + auto p = proc.communicate(Subprocess::writeStdin() | Subprocess::readStdout(), data); EXPECT_EQ(folly::format("{}\n", numLines).str(), p.first); proc.waitChecked(); @@ -100,11 +97,9 @@ TEST(CommunicateSubprocessTest, Duplex) { const int bytes = 10 << 20; std::string line(bytes, 'x'); - Subprocess::Options options; - options.stdin(Subprocess::PIPE).stdout(Subprocess::PIPE); - - Subprocess proc("tr a-z A-Z", options); - auto p = proc.communicate(Subprocess::WRITE_STDIN | Subprocess::READ_STDOUT, + Subprocess proc("tr a-z A-Z", + Subprocess::pipeStdin() | Subprocess::pipeStdout()); + auto p = proc.communicate(Subprocess::writeStdin() | Subprocess::readStdout(), line); EXPECT_EQ(bytes, p.first.size()); EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X')); -- 2.34.1