dlog_logger: add a dedicated signal handler thread

Change-Id: I026304f8023bd236d30f30bb356ccb99adbc4d52
Signed-off-by: Michal Bloch <m.bloch@samsung.com>

diff --git a/src/logger/logger.c b/src/logger/logger.c
index 651232a..5ab57ce 100644 (file)
--- a/src/logger/logger.c
+++ b/src/logger/logger.c
@@ -39,9 +39,6 @@
  * @{
  */
 
-/** global state when logger is not interrupted by any handled signals */
-static volatile sig_atomic_t g_logger_run = 1;
-
 static const int DEFAULT_EPOLL_TIME_MS = 1000;
 
 static const int DEFAULT_LAZY_POLLING_TOTAL_MS = 0;
@@ -539,7 +536,7 @@ void logger_add_writer(struct logger *l, struct writer *wr)
 #ifndef UNIT_TEST
 static
 #endif
-int logger_create(struct logger_config_data *data, struct logger *l)
+int logger_create(struct logger_config_data *data, struct logger *l, struct signal_markup *sigmar)
 {
        memset(l, 0, sizeof *l);
        l->epoll_socket.fd = -1;
@@ -637,31 +634,12 @@ int logger_create(struct logger_config_data *data, struct logger *l)
                        logger_add_writer(l, wr);
                }
 
-       return 0;
-}
-
-/**
- * @brief Handle interrupting/terminating signals
- * @details Clears global flag to stop main loop
- * @param[in] signo signal number
- */
-static void handle_signals(int signo)
-{
-       (void) signo;
-       g_logger_run = 0;
-}
+       add_fd_entity(&l->epoll_socket, &sigmar->wakeup_fde);
+       add_fd_entity(&l->epoll_common, &sigmar->wakeup_fde);
 
-void setup_signals(struct logger *server)
-{
-       struct sigaction action = {
-               .sa_handler = handle_signals,
-               .sa_flags   = 0
-       };
-       sigemptyset(&action.sa_mask);
+       l->sigmar = sigmar;
 
-       static const int handled_signals[] = { SIGINT, SIGTERM };
-       for (unsigned u = 0; u < NELEMS(handled_signals); ++u)
-               sigaction(handled_signals[u], &action, NULL);
+       return 0;
 }
 
 static bool do_logger_one_iteration(struct logger *server, bool *use_lazy_polling)
@@ -697,13 +675,11 @@ static
 #endif
 int do_logger(struct logger *server)
 {
-       setup_signals(server);
-
        /* Note, negative values are applied here, but not in the check in
         * the inner function. This leaves lazy polling enabled forever
         * and is a feature. */
        bool use_lazy_polling = (g_backend.lazy_polling_total != 0);
-       while (g_logger_run)
+       while (!server->sigmar->exit_signal_received)
                if (!do_logger_one_iteration(server, &use_lazy_polling))
                        break;
 
@@ -869,6 +845,12 @@ int prepare_config_data(struct logger_config_data *data)
        }
 
        data->epoll_time = log_config_get_int(&conf, "epoll_time_ms", DEFAULT_EPOLL_TIME_MS);
+
+       /* See the comments near `epoll_wait` in `logger_internal.c`,
+        * the tl;dr is trouble with buffering and signals. */
+       if (data->epoll_time < 0)
+               data->epoll_time = DEFAULT_EPOLL_TIME_MS;
+
        g_backend.lazy_polling_total = 0; // Android Logger backend only, read below
 
        struct qos_module qos = {0, };
@@ -917,7 +899,10 @@ int prepare_config_data(struct logger_config_data *data)
                g_backend.lazy_polling_sleep = log_config_get_int(&conf, "lazy_polling_sleep_ms", DEFAULT_LAZY_POLLING_SLEEP_MS);
 
                /* The total can be 0 (no lazy polling) or negative (infinite),
-                * but the sleep length has to be positive. */
+                * but the sleep length has to be positive. An infinite sleep
+                * not only has a potential to oversleep but also causes issues
+                * with the signal wakeup mechanism. A zero-length sleep would
+                * not let the mode ever end because of our naive timekeeping. */
                if (g_backend.lazy_polling_sleep < 1)
                        g_backend.lazy_polling_sleep = 1;
 
@@ -1135,7 +1120,6 @@ bool early_termination(const struct logger_config_data *data, const struct logge
                if (!reader)
                        continue;
 
-               assert(reader->common.subs != NULL);
                return false;
        }
 
@@ -1158,6 +1142,71 @@ bool early_termination(const struct logger_config_data *data, const struct logge
        return logger->writers == NULL;
 }
 
+static void *signal_thread(void *userdata)
+{
+       struct signal_markup *const sigmar = (struct signal_markup *)userdata;
+
+       int signum;
+       sigwait(&sigmar->sigset, &signum);
+       sigmar->exit_signal_received = true;
+
+       /* Other threads are most likely sleeping on epoll. They're in for
+        * a rude awakening. Now you know why you fear the night. */
+       int r = write(sigmar->wakeup_pipe[1], "wake up, time to die", 1);
+       if (r < 0) {
+               /* This can't realistically fail, but even if somehow
+                * it does, it's not a problem because threads will
+                * wake up regardless after their timeouts complete,
+                * it will just take a moment longer than usual. */
+       }
+
+       return NULL;
+}
+
+static void null_dispatch(struct logger *server, struct epoll_event *event, void *userdata)
+{
+       /* This function handles the wake-up pipe, whose data
+        * is not supposed to be read, just to be acknowledged.
+        * Consuming would prevent other threads from seeing it. */
+}
+
+int setup_signals(struct signal_markup *sigmar)
+{
+       sigmar->exit_signal_received = false;
+
+       /* Broken pipes are somewhat normal, that's what happens when
+        * a client quits (for example a dlogutil instance is manually
+        * closed via Ctrl+C). */
+       signal(SIGPIPE, SIG_IGN);
+
+       /* The pipe is going to be used to notify other threads about the
+        * signal, since that is the only way to interrupt a poll call. */
+       int r = pipe(sigmar->wakeup_pipe);
+       if (r < 0)
+               return r;
+       init_fd_entity(&sigmar->wakeup_fde, null_dispatch, NULL);
+       set_read_fd_entity(&sigmar->wakeup_fde, sigmar->wakeup_pipe[0]);
+
+       /* Setup a dedicated signal handling thread to simplify things
+        * and avoid having to handle it potentially everywhere else. */
+       sigemptyset(&sigmar->sigset);
+       sigaddset(&sigmar->sigset, SIGINT);
+       sigaddset(&sigmar->sigset, SIGTERM);
+       r = pthread_sigmask(SIG_BLOCK, &sigmar->sigset, NULL);
+       if (r < 0)
+               goto cleanup;
+       r = pthread_create(&sigmar->thread, NULL, signal_thread, sigmar);
+       if (r < 0)
+               goto cleanup;
+
+       return 0;
+
+cleanup:
+       close(sigmar->wakeup_pipe[0]);
+       close(sigmar->wakeup_pipe[1]);
+       return r;
+}
+
 /**
  * @brief The logger
  * @return 0 on success, nonzero on failure
@@ -1166,9 +1215,13 @@ bool early_termination(const struct logger_config_data *data, const struct logge
  */
 int main(int argc, char **argv)
 {
-       int r, ret;
-
-       signal(SIGPIPE, SIG_IGN);
+       struct signal_markup sigmar;
+       int r = setup_signals(&sigmar);
+       if (r < 0) {
+               errno = -r;
+               printf("Unable to setup signals (%m). Exiting.\n");
+               return DLOG_EXIT_ERR_RUNTIME;
+       }
 
        r = reset_self_privileges();
        if (r < 0) {
@@ -1199,8 +1252,9 @@ int main(int argc, char **argv)
 
        precreate_logctl_file(&data);
 
+       int ret;
        struct logger server;
-       if ((r = logger_create(&data, &server)) < 0) {
+       if ((r = logger_create(&data, &server, &sigmar)) < 0) {
                errno = -r;
                printf("Unable to initialize logger with provided configuration (%m). Exiting.\n");
                ret = DLOG_EXIT_ERR_CONFIG;
@@ -1227,6 +1281,8 @@ int main(int argc, char **argv)
                goto cleanup;
        }
 
+       pthread_join(sigmar.thread, NULL);
+
        ret = DLOG_EXIT_SUCCESS;
 
 cleanup:

diff --git a/src/logger/logger_internal.c b/src/logger/logger_internal.c
index aebbb65..4b89f73 100644 (file)
--- a/src/logger/logger_internal.c
+++ b/src/logger/logger_internal.c
@@ -44,6 +44,13 @@ int handle_epoll_events(struct logger *server, struct epoll_metadata *metadata,
 {
        ensure_epoll_size(&metadata->events, &metadata->events_size, metadata->cnt);
 
+       /* Some writes are buffered on a timer, which requires us to periodically wake up and
+        * check it. This isn't a fundamental problem, the assert mostly serves as a reminder.
+        * The other issue is that we use the self-pipe trick to wake us up if we receive a
+        * signal to terminate the process, but writing to the pipe can nominally fail, leaving
+        * a thread sleeping forever. This isn't really realistic but doesn't hurt to check. */
+       assert(timeout >= 0);
+
        int nfds = epoll_wait(metadata->fd, metadata->events, metadata->events_size, timeout);
        if (nfds < 0)
                return errno == EINTR ? 0 : -errno;
@@ -65,6 +72,8 @@ int sleep_while_handling_socket(struct logger *server, struct epoll_metadata *me
                int r = handle_epoll_events(server, metadata, timeout);
                if (r < 0)
                        return r;
+               if (server->sigmar->exit_signal_received)
+                       return 0;
                if (clock_gettime(CLOCK_MONOTONIC, &ts_current))
                        return -errno;
        } while (

diff --git a/src/logger/logger_internal.h b/src/logger/logger_internal.h
index f98a949..6b00247 100644 (file)
--- a/src/logger/logger_internal.h
+++ b/src/logger/logger_internal.h
@@ -136,6 +136,14 @@ extern struct backend_data {
  *
  * Of course, the null backend has no relevant data structures. */
 
+struct signal_markup {
+       pthread_t thread;
+       sigset_t sigset;
+       _Atomic _Bool exit_signal_received;
+
+       struct fd_entity wakeup_fde;
+       int wakeup_pipe[2];
+};
 
 struct logger {
        struct epoll_metadata epoll_common;
@@ -146,6 +154,7 @@ struct logger {
        struct log_buffer*    buffers[LOG_ID_MAX];
        struct buf_params     buf_params;
        struct qos_module*    qos;
+       struct signal_markup* sigmar;
        list_head             compressed_memories;
 };