From 0624562e3e1ae24fd1df66e3e38afca7b0f1c70f Mon Sep 17 00:00:00 2001
From: Tom Smeding <tom.smeding@gmail.com>
Date: Tue, 18 Sep 2018 00:06:29 +0200
Subject: Fix bugs

- Cleanup open file descriptors properly
- Use cout, not cerr
- Better error logging, including when a program is killed by a signal
---
 main.cpp    |  5 +++-
 process.cpp | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 process.h   | 11 +++++++--
 3 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/main.cpp b/main.cpp
index ecf48df..edcafb1 100644
--- a/main.cpp
+++ b/main.cpp
@@ -313,7 +313,10 @@ match_done:
 		bool success = procs[i].writeLine("Stop");
 		procs[i].unStop();
 		if (success) usleep(10000);
-		procs[i].terminate();
+		int ret = procs[i].terminate();
+		if (ret != 0 && ret != 1009) {
+			cout << "Player " << i+1 << " exited with code " << ret << endl;
+		}
 	}
 
 	gMultiLog.append(logId, mres.describe(p1, p2));
diff --git a/process.cpp b/process.cpp
index d245449..41325c3 100644
--- a/process.cpp
+++ b/process.cpp
@@ -52,9 +52,11 @@ void Process::run() {
 		dup2(child_out, STDOUT_FILENO);
 		close(infd);
 		close(outfd);
+		close(child_in);
+		close(child_out);
 
 		execlp(execname.data(), execname.data(), NULL);
-		cerr << endl << "ERROR: Error executing player file '" << execname << "'" << endl;
+		cout << endl << "ERROR: Error executing player file '" << execname << "'" << endl;
 		exit(255);
 	}
 
@@ -63,19 +65,58 @@ void Process::run() {
 	close(child_out);
 }
 
-void Process::wait() {
-	if (pid == -1) return;
+int Process::waitPoll() {
+	if (pid == -1) return exitStatus;
+
+	while (true) {
+		int status;
+		int ret = waitpid(pid, &status, WNOHANG);
+		if (ret < 0) {
+			if (errno == ECHILD) return exitStatus;
+			perror("waitpid");
+			return -1;
+		}
+		if (ret == 0) {
+			// Nothing changed yet
+			return -1;
+		}
+
+		if (WIFEXITED(status)) {
+			cleanup();
+			exitStatus = WEXITSTATUS(status);
+			return exitStatus;
+		}
+
+		if (WIFSIGNALED(status)) {
+			cleanup();
+			exitStatus = 1000 + WTERMSIG(status);
+			return exitStatus;
+		}
+	}
+}
+
+int Process::wait() {
+	if (pid == -1) return exitStatus;
+
 	while (true) {
 		int status;
 		if (waitpid(pid, &status, 0) < 0) {
 			if (errno == EINTR) continue;
-			if (errno == ECHILD) break;
+			if (errno == ECHILD) return exitStatus;
 			perror("waitpid");
-			break;
+			return -1;
 		}
+
 		if (WIFEXITED(status)) {
-			pid = -1;
-			break;
+			cleanup();
+			exitStatus = WEXITSTATUS(status);
+			return exitStatus;
+		}
+
+		if (WIFSIGNALED(status)) {
+			cleanup();
+			exitStatus = 1000 + WTERMSIG(status);
+			return exitStatus;
 		}
 	}
 }
@@ -101,7 +142,7 @@ bool Process::writeLine(const string_view line) {
 		ssize_t nw = write(infd, str.data() + cursor, str.size() - cursor);
 		if (nw < 0) {
 			if (errno == EINTR) continue;
-			perror("write");
+			// perror("write");
 			return false;
 		}
 		cursor += nw;
@@ -112,7 +153,7 @@ bool Process::writeLine(const string_view line) {
 
 optional<string> Process::readLine() {
 	if (pid == -1) {
-		cerr << "-- readLine on pid==-1 --" << endl;
+		cout << "-- readLine on pid==-1 --" << endl;
 		return nullopt;
 	}
 
@@ -132,7 +173,7 @@ optional<string> Process::readLine() {
 			return nullopt;
 		}
 		if (nr == 0) {  // EOF
-			cerr << "-- eof in readLine --" << endl;
+			cout << "-- eof in readLine --" << endl;
 			return nullopt;
 		}
 		s.resize(nr);
@@ -148,16 +189,27 @@ optional<string> Process::readLine() {
 	}
 }
 
-void Process::terminate() {
+int Process::terminate() {
 	// TODO: send only SIGTERM, then wait a little while, then send
 	// SIGKILL if necessary
 	if (pid != -1) {
-		kill(pid, SIGKILL);  // force kill
-		wait();
-		pid = -1;
+		int ret = waitPoll();
+		if (ret == -1) {
+			kill(pid, SIGKILL);  // force kill
+			ret = wait();
+		}
+		cleanup();
+		return ret;
 	}
+	return exitStatus;
 }
 
 pid_t Process::getPid() const {
 	return pid;
 }
+
+void Process::cleanup() {
+	pid = -1;
+	close(infd);
+	close(outfd);
+}
diff --git a/process.h b/process.h
index 900a0b6..24ff4b0 100644
--- a/process.h
+++ b/process.h
@@ -14,8 +14,12 @@ class Process {
 	pid_t pid = -1;
 	int infd = -1, outfd = -1;
 
+	int exitStatus = -1;
+
 	string readBuf;
 
+	void cleanup();
+
 public:
 	Process(const string_view execname);
 	Process(const Process&) = delete;
@@ -24,10 +28,13 @@ public:
 	void redirectStderr(const string_view fname);
 
 	void run();
-	void wait();
 	void stop();
 	void unStop();
-	void terminate();
+	// These return -1 if an error occurred, and the process return value otherwise.
+	// If the process was killed by a signal, (1000 + signal) is returned.
+	int wait();
+	int waitPoll();  // Also returns -1 if process hasn't finished yet
+	int terminate();
 
 	pid_t getPid() const;
 
-- 
cgit v1.2.3