Make start_ps_process more C++ish and prevent it throwing exceptions

The function already has a status result so it doesn't make much
sense to allow exceptions to propogate out of it.

diff --git a/service.cc b/service.cc
index 9216bd07a17fa47dc90a06522974cc322c7244b3..6bc34e1052ea1c3d7453aada02bc3bd91f155285 100644 (file)
--- a/service.cc
+++ b/service.cc
@@ -3,6 +3,7 @@
 #include <cerrno>
 #include <sstream>
 #include <iterator>
+#include <memory>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -249,15 +250,20 @@ void ServiceRecord::failed_to_start()
     }
 }
 
-bool ServiceRecord::start_ps_process()
+bool ServiceRecord::start_ps_process() noexcept
 {
-    return start_ps_process(std::vector<std::string>());
+    try {
+        return start_ps_process(std::vector<std::string>());
+    }
+    catch (std::bad_alloc & bad_alloc_exc) {
+        // TODO log error
+        return false;
+    }
 }
 
-
 // TODO this can currently throw std::bad_alloc, fix that (in the worst case,
 //      return failure instead).
-bool ServiceRecord::start_ps_process(const std::vector<std::string> &pargs)
+bool ServiceRecord::start_ps_process(const std::vector<std::string> &pargs) noexcept
 {
     // In general, you can't tell whether fork/exec is successful. We use a pipe to communicate
     // success/failure from the child to the parent. The pipe is set CLOEXEC so a successful
@@ -276,37 +282,12 @@ bool ServiceRecord::start_ps_process(const std::vector<std::string> &pargs)
         return false;
     }
     
-    // TODO make sure pipefd's are not 0/1/2 (STDIN/OUT/ERR) - if they are, dup them
-    // until they are not.
-    
-    pid_t forkpid = fork();
-    if (forkpid == -1) {
-        // TODO log error
-        close(pipefd[0]);
-        close(pipefd[1]);
-        return false;
-    }
-    
-    if (forkpid == 0) {
-        // Child process
-        ev_default_destroy(); // won't need that on this side, free up fds.
-        
-        // Re-set stdin, stdout, stderr
-        close(0); close(1); close(2);
-        string logfile = this->logfile;
-        if (logfile.length() == 0) {
-            logfile = "/dev/null";
-        }
-        
-        // TODO rethink this logic. If we open it at not-0, shouldn't we just dup it to 0?:
-        if (open("/dev/null", O_RDONLY) == 0) {
-          // stdin = 0. That's what we should have; proceed with opening
-          // stdout and stderr.
-          open(logfile.c_str(), O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR);
-          dup2(1, 2);
-        }
-        
-        const char ** args = new const char *[num_args + pargs.size() + 1];
+    // Set up the argument array and other data now (before fork), in case memory allocation fails.
+
+    try {
+        //auto argsv = std::vector<const char *>(num_args + pargs.size() + 1);
+        auto argsv = std::vector<const char *>(num_args + pargs.size() + 1);
+        auto args = argsv.data();
         int i;
         for (i = 0; i < num_args; i++) {
             args[i] = exec_arg_parts[i];
@@ -317,36 +298,73 @@ bool ServiceRecord::start_ps_process(const std::vector<std::string> &pargs)
         }
         args[i] = nullptr;
         
-        execvp(exec_arg_parts[0], const_cast<char **>(args));
-        
-        // If we got here, the exec failed:        
-        int exec_status = errno;
-        write(pipefd[1], &exec_status, sizeof(int));
-        exit(0);
-    }
-    else {
-        // Parent process
-        close(pipefd[1]); // close the 'other end' fd
-
-        int exec_status;        
-        if (read(pipefd[0], &exec_status, sizeof(int)) == 0) {
-            // pipe closed; success
-            pid = forkpid;
+        string logfile = this->logfile;
+        if (logfile.length() == 0) {
+            logfile = "/dev/null";
+        }
 
-            // Add a process listener so we can detect when the
-            // service stops
-            ev_child_init(&child_listener, process_child_callback, pid, 0);
-            child_listener.data = this;
-            ev_child_start(ev_default_loop(EVFLAG_AUTO), &child_listener);
+        // TODO make sure pipefd's are not 0/1/2 (STDIN/OUT/ERR) - if they are, dup them
+        // until they are not.
 
-            close(pipefd[0]);
-            return true;
-        }
-        else {
+        pid_t forkpid = fork();
+        if (forkpid == -1) {
             // TODO log error
             close(pipefd[0]);
+            close(pipefd[1]);
             return false;
         }
+
+        if (forkpid == 0) {
+            // Child process. Must not allocate memory (or otherwise risk throwing any exception)
+            // from here until exit().
+            ev_default_destroy(); // won't need that on this side, free up fds.
+
+            // Re-set stdin, stdout, stderr
+            close(0); close(1); close(2);
+
+            // TODO rethink this logic. If we open it at not-0, shouldn't we just dup it to 0?:
+            if (open("/dev/null", O_RDONLY) == 0) {
+              // stdin = 0. That's what we should have; proceed with opening
+              // stdout and stderr.
+              open(logfile.c_str(), O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR);
+              dup2(1, 2);
+            }
+
+            execvp(exec_arg_parts[0], const_cast<char **>(args));
+
+            // If we got here, the exec failed:
+            int exec_status = errno;
+            write(pipefd[1], &exec_status, sizeof(int));
+            exit(0);
+        }
+        else {
+            // Parent process
+            close(pipefd[1]); // close the 'other end' fd
+
+            int exec_status;
+            if (read(pipefd[0], &exec_status, sizeof(int)) == 0) {
+                // pipe closed; success
+                pid = forkpid;
+
+                // Add a process listener so we can detect when the
+                // service stops
+                ev_child_init(&child_listener, process_child_callback, pid, 0);
+                child_listener.data = this;
+                ev_child_start(ev_default_loop(EVFLAG_AUTO), &child_listener);
+
+                close(pipefd[0]);
+                return true;
+            }
+            else {
+                // TODO log error
+                close(pipefd[0]);
+                return false;
+            }
+        }
+    }
+    catch (std::bad_alloc &bad_alloc_exc) {
+        // TODO log error
+        return false;
     }
 }
 

diff --git a/service.h b/service.h
index a8c34da30579d217dd290274f2b0cf1e32e68b73..f47f73a612f684eb57ed3fb4f4150d4b679ac425 100644 (file)
--- a/service.h
+++ b/service.h
@@ -175,8 +175,8 @@ class ServiceRecord
     void failed_dependency();
     
     // For process services, start the process, return true on success
-    bool start_ps_process();
-    bool start_ps_process(const std::vector<std::string> &args);
+    bool start_ps_process() noexcept;
+    bool start_ps_process(const std::vector<std::string> &args) noexcept;
    
     // Callback from libev when a child process dies
     static void process_child_callback(struct ev_loop *loop, struct ev_child *w,