From d37c20f467d3cf0f176426060c7b173f3347bfdf Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Wed, 24 Feb 2021 23:43:03 +0100 Subject: [PATCH 1/9] Terminate children in the same process group when monitored PID dies Really kill them, our monitored process may be about to restart, so we don't want any unintended side effects from lingering children in prior instances. Signed-off-by: Joachim Wiberg --- src/service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/service.c b/src/service.c index eaa3d551..f82dde8c 100644 --- a/src/service.c +++ b/src/service.c @@ -1172,7 +1172,7 @@ void service_monitor(pid_t lost, int status) } /* Terminate any children in the same proess group, e.g. logit */ - kill(-svc->pid, SIGTERM); + kill(-svc->pid, SIGKILL); /* No longer running, update books. */ svc->start_time = svc->pid = 0; From 6cdcacf20b58805aedbc29a4a9b07c5d62f58362 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Wed, 24 Feb 2021 23:45:14 +0100 Subject: [PATCH 2/9] Refactor, shared cleanup fn for service_kill() and service_monitor() Signed-off-by: Joachim Wiberg --- src/service.c | 56 ++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/service.c b/src/service.c index f82dde8c..1c98acc1 100644 --- a/src/service.c +++ b/src/service.c @@ -620,6 +620,19 @@ static void service_kill(svc_t *svc) print(2, NULL); } +/* + * Clean up any lingering state from dead/killed services + */ +static void service_cleanup(svc_t *svc) +{ + char *fn; + + fn = pid_file(svc); + if (remove(fn) && errno != ENOENT) + logit(LOG_CRIT, "Failed removing service %s pidfile %s", + basename(svc->cmd), fn); +} + /** * service_stop - Stop service * @svc: Service to stop @@ -629,7 +642,7 @@ static void service_kill(svc_t *svc) */ static int service_stop(svc_t *svc) { - int res = 0; + int rc = 0; if (!svc) return 1; @@ -657,25 +670,18 @@ static int service_stop(svc_t *svc) print_desc("Stopping ", svc->desc); if (!svc_is_sysv(svc)) { - if (svc->pid <= 1) - goto cleanup; + if (svc->pid > 1) { + /* Kill all children in the same proess group, e.g. logit */ + rc = kill(-svc->pid, svc->sighalt); - res = kill(-svc->pid, svc->sighalt); + /* PID lost or forking process never really started */ + if (rc == -1 && ESRCH == errno) + service_cleanup(svc); + } else + service_cleanup(svc); - /* PID lost or forking process never really started */ - if (res == -1 && ESRCH == errno) { - char *fn; - - /* XXX: Same clenup as done by service_monitor(), refactor */ - cleanup: - fn = pid_file(svc); - if (remove(fn) && errno != ENOENT) - logit(LOG_CRIT, "Failed removing service %s pidfile %s", - basename(svc->cmd), fn); - - /* No longer running, update books. */ - svc->start_time = svc->pid = 0; - } + /* No longer running, update books. */ + svc->start_time = svc->pid = 0; } else { char *args[] = { svc->cmd, "stop", NULL }; pid_t pid; @@ -689,18 +695,18 @@ static int service_stop(svc_t *svc) break; case -1: _pe("Failed fork() to call sysv script '%s stop'", svc->cmd); - res = 1; + rc = 1; break; default: - res = WEXITSTATUS(complete(svc->cmd, pid)); + rc = WEXITSTATUS(complete(svc->cmd, pid)); break; } } if (runlevel != 1) - print_result(res); + print_result(rc); - return res; + return rc; } /** @@ -1159,11 +1165,7 @@ void service_monitor(pid_t lost, int status) /* Try removing PID file (in case service does not clean up after itself) */ if (svc_is_daemon(svc)) { - char *fn; - - fn = pid_file(svc); - if (remove(fn) && errno != ENOENT) - logit(LOG_CRIT, "Failed removing service %s pidfile %s", basename(svc->cmd), fn); + service_cleanup(svc); } else if (svc_is_runtask(svc)) { if (WIFEXITED(status) && !WEXITSTATUS(status)) svc->started = 1; From 477b50b7a6010e8a7e41f566128511b8faa79339 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Wed, 24 Feb 2021 23:52:06 +0100 Subject: [PATCH 3/9] Check return value from kill(pid, SIGHUP), maybe lost pid This patch handles a corner case when Finit may not have detected a supervised process has died. When a user calls `initctl restart foo` we now send such lost PIDs to the service_monitor() for restart. Signed-off-by: Joachim Wiberg --- src/service.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/service.c b/src/service.c index 1c98acc1..0247b63d 100644 --- a/src/service.c +++ b/src/service.c @@ -722,6 +722,7 @@ static int service_stop(svc_t *svc) static int service_restart(svc_t *svc) { int do_progress = 1; + pid_t lost = 0; int rc; /* Ignore if finit is SIGSTOP'ed */ @@ -748,19 +749,26 @@ static int service_restart(svc_t *svc) logit(LOG_CONSOLE | LOG_NOTICE, "Restarting %s[%d], sending SIGHUP ...", svc_ident(svc, NULL, 0), svc->pid); rc = kill(svc->pid, SIGHUP); + if (rc == -1 && errno == ESRCH) { + /* nobody home, reset internal state machine */ + lost = svc->pid; + } else { + /* Declare we're waiting for svc to re-assert/touch its pidfile */ + svc_starting(svc); - /* Declare we're waiting for svc to re-assert/touch its pidfile */ - svc_starting(svc); - - /* Service does not maintain a PID file on its own */ - if (svc_has_pidfile(svc)) { - sched_yield(); - touch(pid_file(svc)); + /* Service does not maintain a PID file on its own */ + if (svc_has_pidfile(svc)) { + sched_yield(); + touch(pid_file(svc)); + } } if (do_progress) print_result(rc); + if (lost) + service_monitor(lost, 0); + return rc; } From 939c6f2f6748cce6e7133a823e6edceb5aab934b Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 00:22:01 +0100 Subject: [PATCH 4/9] Fix regression in displaying progress at runtime We don't want to show progress (starting/stopping/restaring) at runtime, since Finit v3. Many systems hooked up to a console get confused by sudden output, or even ansi escape sequences. This patch drops code added recently which caused a regression in this policy. The resulting code is even more readable. Signed-off-by: Joachim Wiberg --- src/helpers.c | 3 --- src/log.c | 1 - 2 files changed, 4 deletions(-) diff --git a/src/helpers.c b/src/helpers.c index 6104cadf..ad1943b9 100644 --- a/src/helpers.c +++ b/src/helpers.c @@ -185,9 +185,6 @@ char *strip_line(char *line) void enable_progress(int onoff) { - if (debug) - return; - if (onoff) progress_style = progress_onoff; else diff --git a/src/log.c b/src/log.c index ca251c58..dc574c9e 100644 --- a/src/log.c +++ b/src/log.c @@ -85,7 +85,6 @@ void log_debug(void) screen_init(); } log_open(); - enable_progress(1); logit(LOG_NOTICE, "Debug mode %s", debug ? "enabled" : "disabled"); } From cafbb1862634971e1d8ce347287a2dcc70e02cb8 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 00:27:29 +0100 Subject: [PATCH 5/9] plugins: hotplug: log output from udevd/systemd-udevd to syslog Signed-off-by: Joachim Wiberg --- plugins/hotplug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/hotplug.c b/plugins/hotplug.c index 333a0346..b6d0aedb 100644 --- a/plugins/hotplug.c +++ b/plugins/hotplug.c @@ -43,17 +43,17 @@ static void setup(void *arg) path = which("/lib/systemd/systemd-udevd"); if (path) { /* Register udevd as a monitored service */ - snprintf(cmd, sizeof(cmd), "[S12345789] pid:udevd name:udevd %s " + snprintf(cmd, sizeof(cmd), "[S12345789] pid:udevd name:udevd log %s " "-- Device event managing daemon", path); if (service_register(SVC_TYPE_SERVICE, cmd, global_rlimit, NULL)) { _pe("Failed registering %s", path); } else { - snprintf(cmd, sizeof(cmd), ":1 [S] log " + snprintf(cmd, sizeof(cmd), ":1 [S] log " "udevadm trigger -c add -t devices " "-- Requesting device events"); service_register(SVC_TYPE_RUN, cmd, global_rlimit, NULL); - snprintf(cmd, sizeof(cmd), ":2 [S] log " + snprintf(cmd, sizeof(cmd), ":2 [S] log " "udevadm trigger -c add -t subsystems " "-- Requesting subsystem events"); service_register(SVC_TYPE_RUN, cmd, global_rlimit, NULL); From 3d9ee49759ac9b83c5b0ab1b0e9fa40a3be4c781 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 14:23:45 +0100 Subject: [PATCH 6/9] Follow-up to 6cdcacf, fix minor regression, premature clear of pid Signed-off-by: Joachim Wiberg --- src/service.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/service.c b/src/service.c index 0247b63d..7ba261e4 100644 --- a/src/service.c +++ b/src/service.c @@ -631,6 +631,9 @@ static void service_cleanup(svc_t *svc) if (remove(fn) && errno != ENOENT) logit(LOG_CRIT, "Failed removing service %s pidfile %s", basename(svc->cmd), fn); + + /* No longer running, update books. */ + svc->start_time = svc->pid = 0; } /** @@ -679,9 +682,6 @@ static int service_stop(svc_t *svc) service_cleanup(svc); } else service_cleanup(svc); - - /* No longer running, update books. */ - svc->start_time = svc->pid = 0; } else { char *args[] = { svc->cmd, "stop", NULL }; pid_t pid; From 8790fcabf33a87c72056d3ed8cc2b8c233044d1d Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 14:27:01 +0100 Subject: [PATCH 7/9] iwatch: minor, check if file/dir exists before trying to add Signed-off-by: Joachim Wiberg --- src/iwatch.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/iwatch.c b/src/iwatch.c index ee241a66..5d183a8f 100644 --- a/src/iwatch.c +++ b/src/iwatch.c @@ -80,6 +80,10 @@ int iwatch_add(struct iwatch *iw, char *file, uint32_t mask) if (!initialized) return -1; + if (!fexist(file)) { + _d("No such file or directory, skipping %s", file); + return 0; + } _d("Adding new watcher for path %s", file); path = strdup(file); From ea278a6370ef0bf6122cfb6ccae2feebf65e7616 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 14:27:28 +0100 Subject: [PATCH 8/9] plugins: pidfile: simplify, use constructs from src/conf.c Signed-off-by: Joachim Wiberg --- plugins/pidfile.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/plugins/pidfile.c b/plugins/pidfile.c index b525952f..4377d736 100644 --- a/plugins/pidfile.c +++ b/plugins/pidfile.c @@ -149,36 +149,26 @@ static void pidfile_handle_dir(struct iwatch *iw, char *dir, char *name, int mas static void pidfile_callback(void *arg, int fd, int events) { + static char ev_buf[8 *(sizeof(struct inotify_event) + NAME_MAX + 1) + 1]; struct inotify_event *ev; ssize_t sz, off; - size_t buflen = 8 *(sizeof(struct inotify_event) + NAME_MAX + 1) + 1; - char *buf; - buf= malloc(buflen); - if (!buf) { - _pe("Failed allocating buffer for inotify events"); - return; - } - - _d("Entering ... reading %zu bytes into ev_buf[]", buflen - 1); - sz = read(fd, buf, buflen - 1); + sz = read(fd, ev_buf, sizeof(ev_buf) - 1); if (sz <= 0) { _pe("invalid inotify event"); - goto done; + return; } - buf[sz] = 0; - _d("Read %zd bytes, processing ...", sz); + ev_buf[sz] = 0; - off = 0; for (off = 0; off < sz; off += sizeof(*ev) + ev->len) { struct iwatch_path *iwp; - ev = (struct inotify_event *)&buf[off]; - - _d("path %s, event: 0x%08x", ev->name, ev->mask); + ev = (struct inotify_event *)&ev_buf[off]; if (!ev->mask) continue; + _d("name %s, event: 0x%08x", ev->name, ev->mask); + /* Find base path for this event */ iwp = iwatch_find_by_wd(&iw_pidfile, ev->wd); if (!iwp) @@ -197,8 +187,6 @@ static void pidfile_callback(void *arg, int fd, int events) if (ev->mask & (IN_CREATE | IN_ATTRIB | IN_MODIFY | IN_MOVED_TO)) pidfile_update_conds(iwp->path, ev->name, ev->mask); } -done: - free(buf); } /* From 7ef25abecf905eee215dbbae6364c216e5b10876 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 25 Feb 2021 14:28:03 +0100 Subject: [PATCH 9/9] Refactor, use new iwatch module for inotify of Finit *.conf files - Use iwatch, modeled after pidfile plugin - Use one .conf watcher for all *.conf paths/files - Use full path of conf file for changes, from realpath(). This fixes a long-standing limitation on unique filenames for services, that absolutely nobody knew about Signed-off-by: Joachim Wiberg --- src/conf.c | 159 +++++++++++++++++++--------------------------------- src/conf.h | 4 +- src/finit.c | 8 +-- 3 files changed, 65 insertions(+), 106 deletions(-) diff --git a/src/conf.c b/src/conf.c index 8cec98d7..e866db7e 100644 --- a/src/conf.c +++ b/src/conf.c @@ -34,6 +34,7 @@ #include "finit.h" #include "cond.h" +#include "iwatch.h" #include "service.h" #include "tty.h" #include "helpers.h" @@ -53,7 +54,9 @@ struct conf_change { char *name; }; -static uev_t w1, w2, w3, w4; +static struct iwatch iw_conf; +static uev_t etcw; + static TAILQ_HEAD(head, conf_change) conf_change_list = TAILQ_HEAD_INITIALIZER(conf_change_list); static int parse_conf(char *file); @@ -680,8 +683,9 @@ int conf_reload(void) for (i = 0; i < gl.gl_pathc; i++) { char *path = gl.gl_pathv[i]; - size_t len; + char *rp = NULL; struct stat st; + size_t len; /* Check that it's an actual file ... beyond any symlinks */ if (lstat(path, &st)) { @@ -697,25 +701,22 @@ int conf_reload(void) /* Check for dangling symlinks */ if (S_ISLNK(st.st_mode)) { - char *rp; - - rp = realpath(path, NULL); + path = rp = realpath(path, NULL); if (!rp) { logit(LOG_WARNING, "Skipping %s, dangling symlink: %s", path, strerror(errno)); continue; } - - free(rp); } /* Check that file ends with '.conf' */ len = strlen(path); - if (len < 6 || strcmp(&path[len - 5], ".conf")) { + if (len < 6 || strcmp(&path[len - 5], ".conf")) _d("Skipping %s, not a valid .conf ... ", path); - continue; - } + else + parse_conf_dynamic(path); - parse_conf_dynamic(path); + if (rp) + free(rp); } globfree(&gl); @@ -742,6 +743,7 @@ static struct conf_change *conf_find(char *file) struct conf_change *node, *tmp; TAILQ_FOREACH_SAFE(node, &conf_change_list, link, tmp) { + _d("file: %s vs changed file: %s", file, node->name); if (string_compare(node->name, file)) return node; } @@ -768,13 +770,15 @@ static void drop_changes(void) drop_change(node); } -static int do_change(char *name, uint32_t mask) +static int do_change(char *dir, char *name, uint32_t mask) { + char path[strlen(dir) + strlen(name) + 2]; struct conf_change *node; - _d("Change detected for %s, mask 0x%08x", name, mask); + snprintf(path, sizeof(path), "%s%s", dir, name); + _d("path: %s", path); - node = conf_find(name); + node = conf_find(path); if (mask & (IN_DELETE | IN_MOVED_FROM)) { drop_change(node); return 0; @@ -789,14 +793,14 @@ static int do_change(char *name, uint32_t mask) if (!node) return 1; - node->name = strdup(name); + node->name = strdup(path); if (!node->name) { free(node); return 1; } - _d("Event registered for %s, mask 0x%x", name, mask); - TAILQ_INSERT_HEAD(&conf_change_list, node,link); + _d("Event registered for %s, mask 0x%x", path, mask); + TAILQ_INSERT_HEAD(&conf_change_list, node, link); return 0; } @@ -811,14 +815,9 @@ int conf_any_change(void) int conf_changed(char *file) { - char *ptr; - if (!file) return 0; - if ((ptr = strrchr(file, '/'))) - file = ++ptr; - if (conf_find(file)) return 1; @@ -829,7 +828,7 @@ static void conf_cb(uev_t *w, void *arg, int events) { static char ev_buf[8 *(sizeof(struct inotify_event) + NAME_MAX + 1) + 1]; struct inotify_event *ev; - ssize_t sz, len; + ssize_t sz, off; sz = read(w->fd, ev_buf, sizeof(ev_buf) - 1); if (sz <= 0) { @@ -837,17 +836,23 @@ static void conf_cb(uev_t *w, void *arg, int events) return; } ev_buf[sz] = 0; - ev = (struct inotify_event *)ev_buf; - if (arg) { - do_change(arg, ev->mask); - return; - } + for (off = 0; off < sz; off += sizeof(*ev) + ev->len) { + struct iwatch_path *iwp; - for (ev = (void *)ev_buf; sz > (ssize_t)sizeof(*ev); - len = sizeof(*ev) + ev->len, ev = (void *)ev + len, sz -= len) { - if (do_change(ev->name, ev->mask)) { - _pe("conf_monitor: Out of memory"); + ev = (struct inotify_event *)&ev_buf[off]; + if (!ev->mask) + continue; + + _d("name %s, event: 0x%08x", ev->name, ev->mask); + + /* Find base path for this event */ + iwp = iwatch_find_by_wd(&iw_conf, ev->wd); + if (!iwp) + continue; + + if (do_change(iwp->path, ev->name, ev->mask)) { + _pe(" Out of memory"); break; } } @@ -858,82 +863,23 @@ static void conf_cb(uev_t *w, void *arg, int events) #endif } -static int add_watcher(uev_ctx_t *ctx, uev_t *w, char *path, uint32_t opt) -{ - struct stat st; - uint32_t mask = IN_CREATE | IN_DELETE | IN_MODIFY | IN_ATTRIB | IN_MOVE; - char *arg = NULL; - int fd, wd; - - if (!ctx) - return 0; - - if (stat(path, &st)) { - _d("No such file or directory, skipping %s", path); - w->fd = -1; - return 0; - } - if (!S_ISDIR(st.st_mode)) { - arg = strrchr(path, '/'); - if (!arg) - arg = path; - else - arg++; - } - - if (w->fd >= 0) - close(w->fd); - - fd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC); - if (fd < 0) { - _pe("Failed creating inotify descriptor"); - w->fd = -1; - return 1; - } - - /* - * Only forward error, don't report error, - * user may not have @path and that's OK - */ - wd = inotify_add_watch(fd, path, mask | opt); - if (wd < 0) { - w->fd = -1; - close(fd); - return 1; - } - - if (uev_io_init(ctx, w, conf_cb, arg, fd, UEV_READ)) { - _pe("Failed setting up I/O callback for %s watcher", path); - w->fd = -1; - close(fd); - return 1; - } - _d("Set up inotify watcher for %s ...", path); - - return 0; -} - /* * Set up inotify watcher and load all *.conf in /etc/finit.d/ */ -int conf_monitor(uev_ctx_t *ctx) +int conf_monitor(void) { int rc = 0; - /* Skip second run, when called from finit.c in rescue mode */ - if (ctx && rescue) - return 0; - /* * If only one watcher fails, that's OK. A user may have only * one of /etc/finit.conf or /etc/finit.d in use, and may also * have or not have symlinks in place. We need to monitor for * changes to either symlink or target. */ - rc += add_watcher(ctx, &w1, FINIT_RCSD, 0); - rc += add_watcher(ctx, &w2, FINIT_RCSD "/available/", IN_DONT_FOLLOW); - rc += add_watcher(ctx, &w3, FINIT_RCSD "/enabled/", 0); - rc += add_watcher(ctx, &w4, FINIT_CONF, 0); + rc += iwatch_add(&iw_conf, FINIT_RCSD, IN_ONLYDIR); + rc += iwatch_add(&iw_conf, FINIT_RCSD "/available/", IN_ONLYDIR | IN_DONT_FOLLOW); + rc += iwatch_add(&iw_conf, FINIT_RCSD "/enabled/", IN_ONLYDIR | IN_DONT_FOLLOW); + rc += iwatch_add(&iw_conf, FINIT_CONF, 0); return rc + conf_reload(); } @@ -941,12 +887,25 @@ int conf_monitor(uev_ctx_t *ctx) /* * Prepare .conf parser and load all .conf files */ -int conf_init(void) +int conf_init(uev_ctx_t *ctx) { - hostname = strdup(DEFHOST); - w1.fd = w2.fd = w3.fd = w4.fd = -1; + int fd; - return conf_monitor(NULL); + /* default hostname */ + hostname = strdup(DEFHOST); + + /* prepare /etc watcher */ + fd = iwatch_init(&iw_conf); + if (fd < 0) + return 1; + + if (uev_io_init(ctx, &etcw, conf_cb, NULL, fd, UEV_READ)) { + _pe("Failed setting up I/O callback for /etc watcher"); + close(fd); + return 1; + } + + return 0; } /** diff --git a/src/conf.h b/src/conf.h index 122c93e3..40d882d7 100644 --- a/src/conf.h +++ b/src/conf.h @@ -34,11 +34,11 @@ extern struct rlimit global_rlimit[]; int str2rlim(char *str); char *rlim2str(int rlim); -int conf_init (void); +int conf_init (uev_ctx_t *ctx); void conf_reload (void); int conf_any_change (void); int conf_changed (char *file); -int conf_monitor (uev_ctx_t *ctx); +int conf_monitor (void); void conf_parse_cmdline (int argc, char *argv[]); int conf_parse_runlevels (char *runlevels); diff --git a/src/finit.c b/src/finit.c index bf4085f6..b4d27a5d 100644 --- a/src/finit.c +++ b/src/finit.c @@ -449,7 +449,7 @@ int main(int argc, char *argv[]) /* * Initialize .conf system and load static /etc/finit.conf. */ - conf_init(); + conf_init(&loop); /* Base FS up, enable standard SysV init signals */ sig_setup(&loop); @@ -458,10 +458,10 @@ int main(int argc, char *argv[]) plugin_run_hooks(HOOK_BASEFS_UP); /* - * Set up inotify watcher for /etc/finit.d and read all .conf - * files to figure out how to bootstrap the system. + * Set up inotify watcher for /etc/finit.conf, /etc/finit.d, and + * their deps, to figure out how to bootstrap the system. */ - conf_monitor(&loop); + conf_monitor(); _d("Starting initctl API responder ..."); api_init(&loop);