From 6f4ea7a89bfa14665dc0c5107509e54105cfba6a Mon Sep 17 00:00:00 2001 From: Davin McCall Date: Sun, 15 Nov 2015 15:42:01 +0000 Subject: [PATCH] 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. --- service.cc | 136 ++++++++++++++++++++++++++++++----------------------- service.h | 4 +- 2 files changed, 79 insertions(+), 61 deletions(-) diff --git a/service.cc b/service.cc index 9216bd0..6bc34e1 100644 --- a/service.cc +++ b/service.cc @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -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()); + try { + return start_ps_process(std::vector()); + } + 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 &pargs) +bool ServiceRecord::start_ps_process(const std::vector &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 &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(num_args + pargs.size() + 1); + auto argsv = std::vector(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 &pargs) } args[i] = nullptr; - execvp(exec_arg_parts[0], const_cast(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(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 a8c34da..f47f73a 100644 --- 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 &args); + bool start_ps_process() noexcept; + bool start_ps_process(const std::vector &args) noexcept; // Callback from libev when a child process dies static void process_child_callback(struct ev_loop *loop, struct ev_child *w, -- 2.25.1