From 613d77b042da71f236d93658acbebda463804779 Mon Sep 17 00:00:00 2001 From: Dan Goodliffe Date: Thu, 20 Feb 2020 00:16:40 +0000 Subject: Refactor ProcessPipes to get rid of that awkward feeling --- libadhocutil/processPipes.cpp | 139 +++++++++++++--------------- libadhocutil/processPipes.h | 29 ++++-- libadhocutil/unittests/testProcessPipes.cpp | 24 ++++- 3 files changed, 110 insertions(+), 82 deletions(-) diff --git a/libadhocutil/processPipes.cpp b/libadhocutil/processPipes.cpp index 0c25045..efdcc88 100644 --- a/libadhocutil/processPipes.cpp +++ b/libadhocutil/processPipes.cpp @@ -5,75 +5,73 @@ #include #include #include -#include "scopeExit.h" +#include "c++11Helpers.h" -// NOLINTNEXTLINE(modernize-concat-nested-namespaces) -namespace AdHoc { -namespace System { - -using PipePair = std::array; -static -void -pipe(PipePair & pipes, ScopeExit & tidyUp) +namespace AdHoc::System { +ProcessPipes::InitPipe +ProcessPipes::pipeSetup(bool setup, bool swap) { - if (::pipe(pipes.data())) { - throw SystemException("pipe(2) failed", strerror(errno), errno); + if (setup) { + PipePair pair; + if (::pipe(&pair.first)) { + throw SystemException("pipe(2) failed", strerror(errno), errno); + } + if (swap) { + std::swap(pair.first, pair.second); + } + return pair; } - tidyUp.onFailure.emplace_back([pipes] { - close(pipes[0]); - close(pipes[1]); - }); + return {}; } -ProcessPipes::ProcessPipes(const std::vector & args, bool i, bool o, bool e) : - in(-1), - out(-1), - error(-1) +void ProcessPipes::closeChild(const InitPipe & pipe) noexcept { - if (args.empty()) { - throw std::invalid_argument("args is empty"); - } - PipePair ipipes { -1, -1 }, opipes { -1, -1 }, epipes { -1, -1 }; - ScopeExit tidyUp; - if (i) { - pipe(ipipes, tidyUp); + if (pipe) { + close(pipe->second); } - if (o) { - pipe(opipes, tidyUp); - } - if (e) { - pipe(epipes, tidyUp); +} + +void ProcessPipes::dupChild(int fd, const InitPipe & pipe) noexcept +{ + if (pipe) { + close(pipe->first); + dup2(pipe->second, fd); } - switch (child = fork()) { - case -1: // fail - throw SystemException("fork(2) failed", strerror(errno), errno); +} + +ProcessPipes::ProcessPipes(const std::vector & args, bool i, bool o, bool e) : + ProcessPipes([&args](){ + if (args.empty()) { + throw std::invalid_argument("args is empty"); + } + return args; + }(), pipeSetup(i, true), pipeSetup(o, false), pipeSetup(e, false)) +{ +} + +ProcessPipes::ProcessPipes(const std::vector & args, InitPipe && ipp, InitPipe && opp, InitPipe && epp) : + child([]() { + if (pid_t p = fork(); p != -1) { + return p; + } + /// LCOV_EXCL_START (fork won't fail) + throw SystemException("fork(2) failed", strerror(errno), errno); + /// LCOV_EXCL_STOP + }()), + in(ipp ? std::make_optional(ipp->first, &close) : OFHandle()), + out(opp ? std::make_optional(opp->first, &close) : OFHandle()), + error(epp ? std::make_optional(epp->first, &close) : OFHandle()) +{ + switch (child) { default: // parent - if (i) { - close(ipipes[0]); - in = ipipes[1]; - } - if (o) { - close(opipes[1]); - out = opipes[0]; - } - if (e) { - close(epipes[1]); - error = epipes[0]; - } + closeChild(ipp); + closeChild(opp); + closeChild(epp); break; case 0: // in child - if (i) { - close(ipipes[1]); - dup2(ipipes[0], 0); - } - if (o) { - close(opipes[0]); - dup2(opipes[1], 1); - } - if (e) { - close(epipes[0]); - dup2(epipes[1], 2); - } + dupChild(STDIN_FILENO, ipp); + dupChild(STDOUT_FILENO, opp); + dupChild(STDERR_FILENO, epp); closeAllOpenFiles(); std::vector buf(args.size() + 1); auto w = buf.begin(); @@ -89,35 +87,32 @@ ProcessPipes::ProcessPipes(const std::vector & args, bool i, bool o } } -ProcessPipes::~ProcessPipes() +int +ProcessPipes::closeIn() { - if (in >= 0) { - close(in); - } - if (out >= 0) { - close(out); - } - if (error >= 0) { - close(error); + if (in) { + in.reset(); + return 0; } + return EBADF; } int ProcessPipes::fdIn() const noexcept { - return in; + return in ? *in : -1; } int ProcessPipes::fdOut() const noexcept { - return out; + return out ? *out : -1; } int ProcessPipes::fdError() const noexcept { - return error; + return error ? *error : -1; } pid_t @@ -127,7 +122,7 @@ ProcessPipes::pid() const noexcept } void -ProcessPipes::closeAllOpenFiles() +ProcessPipes::closeAllOpenFiles() noexcept { rlimit lim { }; getrlimit(RLIMIT_NOFILE, &lim); @@ -143,7 +138,5 @@ ProcessPipes::closeAllOpenFiles() } } } - -} } diff --git a/libadhocutil/processPipes.h b/libadhocutil/processPipes.h index 095ceb7..1b1a8fa 100644 --- a/libadhocutil/processPipes.h +++ b/libadhocutil/processPipes.h @@ -3,10 +3,13 @@ #include #include +#include #include "visibility.h" #include "c++11Helpers.h" +#include "handle.h" namespace AdHoc { + namespace System { /// Spawn a process and attach to its IO handles. @@ -21,9 +24,9 @@ class DLL_PUBLIC ProcessPipes { * @param err Attach to stderr? */ ProcessPipes(const std::vector & args, bool in, bool out, bool err); - /// Standard move/copy support - SPECIAL_MEMBERS_DEFAULT_MOVE_NO_COPY(ProcessPipes); - ~ProcessPipes(); + + /// Close input pipe to process + int closeIn(); /** FD handle to child's stdin. */ [[nodiscard]] int fdIn() const noexcept; @@ -34,12 +37,22 @@ class DLL_PUBLIC ProcessPipes { /** Process id of child. */ [[nodiscard]] pid_t pid() const noexcept; - /** Close all open file handles as determined by rlimit and poll. */ - static void closeAllOpenFiles(); - private: - int in, out, error; - pid_t child; + using PipePair = std::pair; + using InitPipe = std::optional; + + ProcessPipes(const std::vector & args, InitPipe &&, InitPipe &&, InitPipe &&); + + static InitPipe pipeSetup(bool setup, bool swap); + static void closeChild(const InitPipe & child) noexcept; + static void dupChild(int, const InitPipe & child) noexcept; + static void closeAllOpenFiles() noexcept; + + using FHandle = ::AdHoc::Handle; + using OFHandle = std::optional; + const pid_t child; + OFHandle in; + const OFHandle out, error; }; } diff --git a/libadhocutil/unittests/testProcessPipes.cpp b/libadhocutil/unittests/testProcessPipes.cpp index b568984..968ac2a 100644 --- a/libadhocutil/unittests/testProcessPipes.cpp +++ b/libadhocutil/unittests/testProcessPipes.cpp @@ -7,7 +7,7 @@ using namespace AdHoc::System; -BOOST_AUTO_TEST_CASE ( readfind ) +BOOST_AUTO_TEST_CASE( readfind ) { ProcessPipes pp({"/usr/bin/find", rootDir, "-maxdepth", "1"}, false, true, true); BOOST_REQUIRE_EQUAL(pp.fdIn(), -1); @@ -23,3 +23,25 @@ BOOST_AUTO_TEST_CASE ( readfind ) waitpid(pp.pid(), &status, 0); } +BOOST_AUTO_TEST_CASE( readwrite ) +{ + ProcessPipes pp({"/usr/bin/md5sum"}, true, true, false); + BOOST_REQUIRE_NE(pp.fdIn(), -1); + BOOST_REQUIRE_NE(pp.fdOut(), -1); + BOOST_REQUIRE_EQUAL(pp.fdError(), -1); + BOOST_REQUIRE_EQUAL(11, write(pp.fdIn(), "some string", 11)); + BOOST_REQUIRE_EQUAL(0, pp.closeIn()); + std::string out(32, ' '); + BOOST_REQUIRE_EQUAL(32, read(pp.fdOut(), out.data(), 32)); + BOOST_CHECK_EQUAL(out, "5ac749fbeec93607fc28d666be85e73a"); + int status; + waitpid(pp.pid(), &status, 0); +} + +BOOST_AUTO_TEST_CASE( noargs ) +{ + BOOST_CHECK_THROW({ + ProcessPipes({}, false, false, false); + }, std::invalid_argument); +} + -- cgit v1.2.3