Commit 96bd762f authored by Tudor Bosman's avatar Tudor Bosman Committed by Jordan DeLong

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
parent dcbf9149
......@@ -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<std::string, std::string> Subprocess::communicate(
int flags,
const CommunicateFlags& flags,
StringPiece data) {
IOBufQueue dataQueue;
dataQueue.wrapBuffer(data.data(), data.size());
......@@ -554,14 +540,14 @@ std::pair<std::string, std::string> Subprocess::communicate(
}
std::pair<IOBufQueue, IOBufQueue> Subprocess::communicateIOBuf(
int flags,
const CommunicateFlags& flags,
IOBufQueue data) {
std::pair<IOBufQueue, IOBufQueue> 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<IOBufQueue, IOBufQueue> 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.
......
......@@ -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<Options> {
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<int, int> 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<CommunicateFlags> {
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<IOBufQueue, IOBufQueue> communicateIOBuf(
int flags = READ_STDOUT,
const CommunicateFlags& flags = readStdout(),
IOBufQueue data = IOBufQueue());
std::pair<std::string, std::string> communicate(
int flags = READ_STDOUT,
const CommunicateFlags& flags = readStdout(),
StringPiece data = StringPiece());
/**
......@@ -452,6 +483,27 @@ class Subprocess : private boost::noncopyable {
std::vector<PipeInfo> 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_ */
......
......@@ -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<std::string>{ "/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'));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment