From edf882c98c02fbbaa87704ec47e63190976b4b16 Mon Sep 17 00:00:00 2001 From: Davin McCall Date: Mon, 4 Jun 2018 22:09:02 +0100 Subject: [PATCH] proctests: add a new test, currently failing. This also includes more OS abstraction in bp_sys so that we can test different exit statuses. --- src/includes/baseproc-sys.h | 49 +++++++++++++++++++++ src/includes/proc-service.h | 13 +++--- src/proc-service.cc | 60 +++++++++++++------------- src/tests/proctests.cc | 50 ++++++++++++++++++++- src/tests/test-includes/baseproc-sys.h | 59 +++++++++++++++++++++++++ 5 files changed, 194 insertions(+), 37 deletions(-) diff --git a/src/includes/baseproc-sys.h b/src/includes/baseproc-sys.h index 44da762..6bd0de4 100644 --- a/src/includes/baseproc-sys.h +++ b/src/includes/baseproc-sys.h @@ -5,6 +5,11 @@ * for the functions, to avoid calling the real functions and thus allow for unit-level testing. */ +#ifndef BPSYS_INCLUDED +#define BPSYS_INCLUDED + +#include "dasynq.h" + namespace bp_sys { using dasynq::pipe2; @@ -16,4 +21,48 @@ using ::getpgid; using ::tcsetpgrp; using ::getpgrp; +// Wrapper around a POSIX exit status +class exit_status +{ + friend pid_t waitpid(pid_t, exit_status *, int); + + int status; + + public: + exit_status() : status(0) { } + explicit exit_status(int status_p) : status(status_p) { } + + bool did_exit() + { + return WIFEXITED(status); + } + + bool did_exit_clean() + { + return status == 0; + } + + bool was_signalled() + { + return WIFSIGNALED(status); + } + + int get_exit_status() + { + return WEXITSTATUS(status); + } + + int get_term_sig() + { + return WTERMSIG(status); + } +}; + +inline pid_t waitpid(pid_t p, exit_status *statusp, int flags) +{ + return ::waitpid(p, &statusp->status, flags); } + +} + +#endif // BPSYS_INCLUDED diff --git a/src/includes/proc-service.h b/src/includes/proc-service.h index d734c7e..2ebe32e 100644 --- a/src/includes/proc-service.h +++ b/src/includes/proc-service.h @@ -1,5 +1,6 @@ #include +#include "baseproc-sys.h" #include "service.h" // This header defines base_proc_service (base process service) and several derivatives, as well as some @@ -74,7 +75,7 @@ class base_process_service : public service_record pid_t pid = -1; // PID of the process. If state is STARTING or STOPPING, // this is PID of the service script; otherwise it is the // PID of the process itself (process service). - int exit_status = 0; // Exit status, if the process has exited (pid == -1). + bp_sys::exit_status exit_status; // Exit status, if the process has exited (pid == -1). int socket_fd = -1; // For socket-activation services, this is the file // descriptor for the socket. @@ -99,7 +100,7 @@ class base_process_service : public service_record // Called when the process exits. The exit_status is the status value yielded by // the "wait" system call. - virtual void handle_exit_status(int exit_status) noexcept = 0; + virtual void handle_exit_status(bp_sys::exit_status exit_status) noexcept = 0; // Called if an exec fails. virtual void exec_failed(int errcode) noexcept = 0; @@ -207,7 +208,7 @@ class base_process_service : public service_record // Standard process service. class process_service : public base_process_service { - virtual void handle_exit_status(int exit_status) noexcept override; + virtual void handle_exit_status(bp_sys::exit_status exit_status) noexcept override; virtual void exec_failed(int errcode) noexcept override; virtual void exec_succeeded() noexcept override; virtual void bring_down() noexcept override; @@ -229,7 +230,7 @@ class process_service : public base_process_service // Bgproc (self-"backgrounding", i.e. double-forking) process service class bgproc_service : public base_process_service { - virtual void handle_exit_status(int exit_status) noexcept override; + virtual void handle_exit_status(bp_sys::exit_status exit_status) noexcept override; virtual void exec_failed(int errcode) noexcept override; virtual void bring_down() noexcept override; @@ -240,7 +241,7 @@ class bgproc_service : public base_process_service }; // Read the pid-file, return false on failure - pid_result_t read_pid_file(int *exit_status) noexcept; + pid_result_t read_pid_file(bp_sys::exit_status *exit_status) noexcept; public: bgproc_service(service_set *sset, string name, string &&command, @@ -259,7 +260,7 @@ class bgproc_service : public base_process_service // Service which is started and stopped via separate commands class scripted_service : public base_process_service { - virtual void handle_exit_status(int exit_status) noexcept override; + virtual void handle_exit_status(bp_sys::exit_status exit_status) noexcept override; virtual void exec_failed(int errcode) noexcept override; virtual void bring_down() noexcept override; diff --git a/src/proc-service.cc b/src/proc-service.cc index 07ef7ee..d482fde 100644 --- a/src/proc-service.cc +++ b/src/proc-service.cc @@ -97,7 +97,7 @@ dasynq::rearm service_child_watcher::status_change(eventloop_t &loop, pid_t chil base_process_service *sr = service; sr->pid = -1; - sr->exit_status = status; + sr->exit_status = bp_sys::exit_status(status); // Ok, for a process service, any process death which we didn't rig // ourselves is a bit... unexpected. Probably, the child died because @@ -119,30 +119,30 @@ dasynq::rearm service_child_watcher::status_change(eventloop_t &loop, pid_t chil sr->stop_timer_armed = false; } - sr->handle_exit_status(status); + sr->handle_exit_status(bp_sys::exit_status(status)); return dasynq::rearm::NOOP; } -void process_service::handle_exit_status(int exit_status) noexcept +void process_service::handle_exit_status(bp_sys::exit_status exit_status) noexcept { - bool did_exit = WIFEXITED(exit_status); - bool was_signalled = WIFSIGNALED(exit_status); + bool did_exit = exit_status.did_exit(); + bool was_signalled = exit_status.was_signalled(); restarting = false; auto service_state = get_state(); - if (exit_status != 0 && service_state != service_state_t::STOPPING) { + if (exit_status.did_exit_clean() && service_state != service_state_t::STOPPING) { if (did_exit) { log(loglevel_t::ERROR, "Service ", get_name(), " process terminated with exit code ", - WEXITSTATUS(exit_status)); + exit_status.get_exit_status()); } else if (was_signalled) { log(loglevel_t::ERROR, "Service ", get_name(), " terminated due to signal ", - WTERMSIG(exit_status)); + exit_status.get_term_sig()); } } if (service_state == service_state_t::STARTING) { - if (did_exit && WEXITSTATUS(exit_status) == 0) { + if (exit_status.did_exit_clean()) { started(); } else { @@ -180,21 +180,21 @@ void process_service::exec_failed(int errcode) noexcept } } -void bgproc_service::handle_exit_status(int exit_status) noexcept +void bgproc_service::handle_exit_status(bp_sys::exit_status exit_status) noexcept { begin: - bool did_exit = WIFEXITED(exit_status); - bool was_signalled = WIFSIGNALED(exit_status); + bool did_exit = exit_status.did_exit(); + bool was_signalled = exit_status.was_signalled(); auto service_state = get_state(); - if (exit_status != 0 && service_state != service_state_t::STOPPING) { + if (!exit_status.did_exit_clean() && service_state != service_state_t::STOPPING) { if (did_exit) { log(loglevel_t::ERROR, "Service ", get_name(), " process terminated with exit code ", - WEXITSTATUS(exit_status)); + exit_status.get_exit_status()); } else if (was_signalled) { log(loglevel_t::ERROR, "Service ", get_name(), " terminated due to signal ", - WTERMSIG(exit_status)); + exit_status.get_term_sig()); } } @@ -203,7 +203,7 @@ void bgproc_service::handle_exit_status(int exit_status) noexcept if (restarting && service_state == service_state_t::STARTED) { restarting = false; bool need_stop = false; - if ((did_exit && WEXITSTATUS(exit_status) != 0) || was_signalled) { + if ((did_exit && exit_status.get_exit_status() != 0) || was_signalled) { need_stop = true; } else { @@ -236,7 +236,7 @@ void bgproc_service::handle_exit_status(int exit_status) noexcept if (service_state == service_state_t::STARTING) { // POSIX requires that if the process exited clearly with a status code of 0, // the exit status value will be 0: - if (exit_status == 0) { + if (exit_status.did_exit_clean()) { auto pid_result = read_pid_file(&exit_status); switch (pid_result) { case pid_result_t::FAILED: @@ -285,10 +285,10 @@ void bgproc_service::exec_failed(int errcode) noexcept failed_to_start(); } -void scripted_service::handle_exit_status(int exit_status) noexcept +void scripted_service::handle_exit_status(bp_sys::exit_status exit_status) noexcept { - bool did_exit = WIFEXITED(exit_status); - bool was_signalled = WIFSIGNALED(exit_status); + bool did_exit = exit_status.did_exit(); + bool was_signalled = exit_status.was_signalled(); auto service_state = get_state(); // For a scripted service, a termination occurs in one of three main cases: @@ -301,16 +301,16 @@ void scripted_service::handle_exit_status(int exit_status) noexcept // a cancel order via SIGINT: if (interrupting_start) { // We issued a start interrupt, so we expected this failure: - if (did_exit && WEXITSTATUS(exit_status) != 0) { + if (did_exit && exit_status.get_exit_status() != 0) { log(loglevel_t::INFO, "Service ", get_name(), " start cancelled; exit code ", - WEXITSTATUS(exit_status)); + exit_status.get_exit_status()); // Assume that a command terminating normally requires no cleanup: stopped(); } else { if (was_signalled) { log(loglevel_t::INFO, "Service ", get_name(), " start cancelled from signal ", - WTERMSIG(exit_status)); + exit_status.get_term_sig()); } // If the start script completed successfully, or was interrupted via our signal, // we want to run the stop script to clean up: @@ -318,7 +318,7 @@ void scripted_service::handle_exit_status(int exit_status) noexcept } interrupting_start = false; } - else if (did_exit && WEXITSTATUS(exit_status) == 0) { + else if (exit_status.did_exit_clean()) { // We were running the stop script and finished successfully stopped(); } @@ -326,11 +326,11 @@ void scripted_service::handle_exit_status(int exit_status) noexcept // ??? failed to stop! Let's log it as warning: if (did_exit) { log(loglevel_t::WARN, "Service ", get_name(), " stop command failed with exit code ", - WEXITSTATUS(exit_status)); + exit_status.get_exit_status()); } else if (was_signalled) { log(loglevel_t::WARN, "Service ", get_name(), " stop command terminated due to signal ", - WTERMSIG(exit_status)); + exit_status.get_term_sig()); } // Even if the stop script failed, assume that service is now stopped, so that any dependencies // can be stopped. There's not really any other useful course of action here. @@ -339,18 +339,18 @@ void scripted_service::handle_exit_status(int exit_status) noexcept services->process_queues(); } else { // STARTING - if (exit_status == 0) { + if (exit_status.did_exit_clean()) { started(); } else { // failed to start if (did_exit) { log(loglevel_t::ERROR, "Service ", get_name(), " command failed with exit code ", - WEXITSTATUS(exit_status)); + exit_status.get_exit_status()); } else if (was_signalled) { log(loglevel_t::ERROR, "Service ", get_name(), " command terminated due to signal ", - WTERMSIG(exit_status)); + exit_status.get_term_sig()); } failed_to_start(); } @@ -373,7 +373,7 @@ void scripted_service::exec_failed(int errcode) noexcept } bgproc_service::pid_result_t -bgproc_service::read_pid_file(int *exit_status) noexcept +bgproc_service::read_pid_file(bp_sys::exit_status *exit_status) noexcept { const char *pid_file_c = pid_file.c_str(); int fd = open(pid_file_c, O_CLOEXEC); diff --git a/src/tests/proctests.cc b/src/tests/proctests.cc index 3c533cf..2c1a35e 100644 --- a/src/tests/proctests.cc +++ b/src/tests/proctests.cc @@ -14,6 +14,10 @@ extern eventloop_t event_loop; +constexpr static auto REG = dependency_type::REGULAR; +constexpr static auto WAITS = dependency_type::WAITS_FOR; +constexpr static auto MS = dependency_type::MILESTONE; + // Friend interface to access base_process_service private/protected members. class base_process_service_test { @@ -27,7 +31,7 @@ class base_process_service_test static void handle_exit(base_process_service *bsp, int exit_status) { bsp->pid = -1; - bsp->handle_exit_status(exit_status); + bsp->handle_exit_status(bp_sys::exit_status(true, false, exit_status)); } }; @@ -414,6 +418,49 @@ void test_scripted_stop_timeout() sset.remove_service(&p); } +void test_scripted_start_fail() +{ + using namespace std; + + service_set sset; + + string command = "test-command"; + string stopcommand = "stop-command"; + list> command_offsets; + command_offsets.emplace_back(0, command.length()); + std::list depends; + + scripted_service p = scripted_service(&sset, "testscripted", std::move(command), command_offsets, depends); + init_service_defaults(p); + p.set_stop_command(stopcommand, command_offsets); + sset.add_service(&p); + + service_record *s2 = new service_record(&sset, "test-service-2", service_type_t::INTERNAL, {{&p, REG}}); + service_record *s3 = new service_record(&sset, "test-service-3", service_type_t::INTERNAL, {{&p, REG}, {s2, REG}}); + sset.add_service(s2); + sset.add_service(s3); + + s3->start(true); + sset.process_queues(); + + assert(p.get_state() == service_state_t::STARTING); + + base_process_service_test::exec_succeeded(&p); + sset.process_queues(); + base_process_service_test::handle_exit(&p, 0x1); // exit fail + sset.process_queues(); + + // failed to start: + assert(p.get_state() == service_state_t::STOPPED); + assert(s2->get_state() == service_state_t::STOPPED); + assert(s3->get_state() == service_state_t::STOPPED); + + event_loop.active_timers.clear(); + sset.remove_service(&p); + + assert(sset.count_active_services() == 0); +} + #define RUN_TEST(name, spacing) \ std::cout << #name "..." spacing; \ @@ -431,4 +478,5 @@ int main(int argc, char **argv) RUN_TEST(test_proc_smooth_recovery1, ""); RUN_TEST(test_proc_smooth_recovery2, ""); RUN_TEST(test_scripted_stop_timeout, ""); + RUN_TEST(test_scripted_start_fail, " "); } diff --git a/src/tests/test-includes/baseproc-sys.h b/src/tests/test-includes/baseproc-sys.h index 3ca25e0..8a5674a 100644 --- a/src/tests/test-includes/baseproc-sys.h +++ b/src/tests/test-includes/baseproc-sys.h @@ -1,3 +1,7 @@ +#ifndef BPSYS_INCLUDED +#define BPSYS_INCLUDED + +#include #include #include @@ -5,6 +9,7 @@ namespace bp_sys { +// implementations elsewhere: int pipe2(int pipefd[2], int flags); int close(int fd); int kill(pid_t pid, int sig); @@ -30,4 +35,58 @@ inline pid_t getpgrp() return getpid(); } +class exit_status +{ + friend pid_t waitpid(pid_t, exit_status *, int); + + bool did_exit_v; + bool was_signalled_v; + int status; + + public: + exit_status() : did_exit_v(true), was_signalled_v(false), status(0) { } + + // status_p is either the exit status or termination signal: + exit_status(bool did_exit_p, bool was_signalled_p, int status_p) + : did_exit_v(did_exit_p), was_signalled_v(was_signalled_p), status(status_p) + { } + + explicit exit_status(int status_p) + { + throw std::string("initalised exit_status with integer argument"); + } + + bool did_exit() + { + return did_exit_v; + } + + bool did_exit_clean() + { + return did_exit_v && status == 0; + } + + bool was_signalled() + { + return was_signalled_v; + } + + int get_exit_status() + { + return status; + } + + int get_term_sig() + { + return status; + } +}; + +inline pid_t waitpid(pid_t p, exit_status *statusp, int flags) +{ + throw std::string("not implemented"); +} + } + +#endif -- 2.25.1