Rewrite subprocess and add test cases for it

src/cpp-utils/process/subprocess.cpp

@@ -17,31 +17,8 @@ using std::string;
 namespace cpputils {
     //TODO Exception safety
 
-    string Subprocess::call(const string &command) {
-        FILE *subprocessOutput = _call(command);
-
-        string result;
-        char buffer[1024];
-        while(fgets(buffer, sizeof(buffer), subprocessOutput) != nullptr) {
-            result += buffer;
-        }
-
-        auto returncode = pclose(subprocessOutput);
-        if(WEXITSTATUS(returncode) != 0) {
-            throw std::runtime_error("Subprocess \""+command+"\" exited with code "+std::to_string(WEXITSTATUS(returncode)));
-        }
-
-        return result;
-    }
-
-    int Subprocess::callAndGetReturnCode(const string &command) {
-        FILE *subprocess = _call(command);
-
-        auto returncode = pclose(subprocess);
-        return WEXITSTATUS(returncode);
-    }
-
-    FILE *Subprocess::_call(const string &command) {
+    namespace {
+    FILE *_call(const string &command) {
         FILE *subprocess = popen(command.c_str(), openmode);
         if (!subprocess)
         {
@@ -49,4 +26,43 @@ namespace cpputils {
         }
         return subprocess;
     }
+
+    string _getOutput(FILE *subprocess) {
+        string output;
+        char buffer[1024];
+        while(fgets(buffer, sizeof(buffer), subprocess) != nullptr) {
+            output += buffer;
+        }
+        return output;
+    }
+
+    int _close(FILE *subprocess) {
+        auto returncode = pclose(subprocess);
+        if(returncode == -1) {
+            throw std::runtime_error("Error calling pclose. Errno: " + std::to_string(errno));
+        }
+        if (WIFEXITED(returncode) == 0) {
+            // WEXITSTATUS is only valud if WIFEXITED is 0.
+            throw std::runtime_error("WIFEXITED returned " + std::to_string(WIFEXITED(returncode)));
+        }
+        return WEXITSTATUS(returncode);
+    }
+    }
+
+    SubprocessResult Subprocess::call(const string &command) {
+        FILE *subprocess = _call(command);
+        string output = _getOutput(subprocess);
+        int exitcode = _close(subprocess);
+
+        return SubprocessResult {output, exitcode};
+    }
+
+    SubprocessResult Subprocess::check_call(const string &command) {
+        auto result = call(command);
+        if(result.exitcode != 0) {
+            throw SubprocessError("Subprocess \""+command+"\" exited with code "+std::to_string(result.exitcode));
+        }
+        return result;
+    }
+
 }

src/cpp-utils/process/subprocess.h

@@ -3,16 +3,25 @@
 #define MESSMER_CPPUTILS_PROCESS_SUBPROCESS_H
 
 #include <string>
+#include <stdexcept>
 #include "../macros.h"
 
 namespace cpputils {
+    struct SubprocessResult final {
+        std::string output;
+        int exitcode;
+    };
+
+    struct SubprocessError final : public std::runtime_error {
+        SubprocessError(std::string msg): std::runtime_error(std::move(msg)) {}
+    };
+
     //TODO Test
     class Subprocess final {
     public:
-        static std::string call(const std::string &command);
-        static int callAndGetReturnCode(const std::string &command);
+        static SubprocessResult call(const std::string &command);
+        static SubprocessResult check_call(const std::string &command);
     private:
-        static FILE* _call(const std::string &command);
 
         DISALLOW_COPY_AND_ASSIGN(Subprocess);
     };

test/cpp-utils/CMakeLists.txt

@@ -15,6 +15,7 @@ set(SOURCES
     pointer/unique_ref_include_test.cpp
     process/daemonize_include_test.cpp
     process/subprocess_include_test.cpp
+    process/SubprocessTest.cpp
     tempfile/TempFileTest.cpp
     tempfile/TempFileIncludeTest.cpp
     tempfile/TempDirIncludeTest.cpp

test/cpp-utils/process/SubprocessTest.cpp

@@ -0,0 +1,89 @@
+#include <cpp-utils/process/subprocess.h>
+#include <gtest/gtest.h>
+
+using cpputils::Subprocess;
+using cpputils::SubprocessError;
+
+TEST(SubprocessTest, CheckCall_success_output) {
+    EXPECT_EQ("hello\n", Subprocess::check_call("echo hello").output);
+}
+
+TEST(SubprocessTest, CheckCall_successwithemptyoutput_output) {
+    EXPECT_EQ("", Subprocess::check_call("exit 0").output);
+}
+
+TEST(SubprocessTest, CheckCall_success_exitcode) {
+    EXPECT_EQ(0, Subprocess::check_call("echo hello").exitcode);
+}
+
+TEST(SubprocessTest, CheckCall_successwithemptyoutput_exitcode) {
+    EXPECT_EQ(0, Subprocess::check_call("exit 0").exitcode);
+}
+
+TEST(SubprocessTest, CheckCall_error) {
+    EXPECT_THROW(
+      Subprocess::check_call("exit 1"),
+      SubprocessError
+    );
+}
+
+TEST(SubprocessTest, CheckCall_error5) {
+    EXPECT_THROW(
+      Subprocess::check_call("exit 5"),
+      SubprocessError
+    );
+}
+
+TEST(SubprocessTest, CheckCall_errorwithoutput) {
+    EXPECT_THROW(
+      Subprocess::check_call("echo hello; exit 1"),
+      SubprocessError
+    );
+}
+
+TEST(SubprocessTest, CheckCall_error5withoutput) {
+    EXPECT_THROW(
+      Subprocess::check_call("echo hello; exit 5"),
+      SubprocessError
+    );
+}
+
+TEST(SubprocessTest, Call_success_exitcode) {
+    EXPECT_EQ(0, Subprocess::call("echo hello").exitcode);
+}
+
+TEST(SubprocessTest, Call_success_output) {
+    EXPECT_EQ("hello\n", Subprocess::call("echo hello").output);
+}
+
+TEST(SubprocessTest, Call_error_exitcode) {
+    EXPECT_EQ(1, Subprocess::call("exit 1").exitcode);
+}
+
+TEST(SubprocessTest, Call_error_output) {
+    EXPECT_EQ("", Subprocess::call("exit 1").output);
+}
+
+TEST(SubprocessTest, Call_error5_exitcode) {
+    EXPECT_EQ(5, Subprocess::call("exit 5").exitcode);
+}
+
+TEST(SubprocessTest, Call_error5_output) {
+    EXPECT_EQ("", Subprocess::call("exit 1").output);
+}
+
+TEST(SubprocessTest, Call_errorwithoutput_output) {
+    EXPECT_EQ("hello\n", Subprocess::call("echo hello; exit 1").output);
+}
+
+TEST(SubprocessTest, Call_errorwithoutput_exitcode) {
+    EXPECT_EQ(1, Subprocess::call("echo hello; exit 1").exitcode);
+}
+
+TEST(SubprocessTest, Call_error5withoutput_output) {
+    EXPECT_EQ("hello\n", Subprocess::call("echo hello; exit 5").output);
+}
+
+TEST(SubprocessTest, Call_error5withoutput_exitcode) {
+    EXPECT_EQ(5, Subprocess::call("echo hello; exit 5").exitcode);
+}

test/cryfs-cli/testutils/CliTest.h

@@ -67,9 +67,9 @@ public:
             int returncode = -1;
             while (returncode != 0) {
 #ifdef __APPLE__
-                returncode = cpputils::Subprocess::callAndGetReturnCode(std::string("umount ") + mountDir.c_str() + " 2>/dev/null");
+                returncode = cpputils::Subprocess::call(std::string("umount ") + mountDir.c_str() + " 2>/dev/null").exitcode;
 #else
-                returncode = cpputils::Subprocess::callAndGetReturnCode(std::string("fusermount -u ") + mountDir.c_str() + " 2>/dev/null");
+                returncode = cpputils::Subprocess::call(std::string("fusermount -u ") + mountDir.c_str() + " 2>/dev/null").exitcode;
 #endif
                 //std::this_thread::sleep_for(std::chrono::milliseconds(50)); // TODO Is this the test case duration? Does a shorter interval make the test case faster?
             }