Refactor ProcessPipes to get rid of that awkward feeling

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 <sys/resource.h>
 #include <stdexcept>
 #include <sys.h>
-#include "scopeExit.h"
+#include "c++11Helpers.h"
 
-// NOLINTNEXTLINE(modernize-concat-nested-namespaces)
-namespace AdHoc {
-namespace System {
-
-using PipePair = std::array<int, 2>;
-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<std::string> & 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<std::string> & 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<std::string> & 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<FHandle>(ipp->first, &close) : OFHandle()),
+	out(opp ? std::make_optional<FHandle>(opp->first, &close) : OFHandle()),
+	error(epp ? std::make_optional<FHandle>(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<char *> buf(args.size() + 1);
 			auto w = buf.begin();
@@ -89,35 +87,32 @@ ProcessPipes::ProcessPipes(const std::vector<std::string> & 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 <vector>
 #include <string>
+#include <optional>
 #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<std::string> & 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<int, int>;
+		using InitPipe = std::optional<PipePair>;
+
+		ProcessPipes(const std::vector<std::string> & 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<int, int(*)(int)>;
+		using OFHandle = std::optional<FHandle>;
+		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);
+}
+