.conf: don't rely on mtime to determine if service needs reloading

Until now Finit has used mtime to determine if a file has been changed,
or simply touched to request reload, when `initctl reload` is called.
Using mtime for this purpose is a monumentally bad idea since time can
be changed at any point: a user may adjust the time, NTP continously
tunes the clock, and time stamps are stored as the walltime.  So if we
compare timestamps against the last time we (booted or) did reload, we
would miss .conf file changes.

This patch replaces the mtime mistake with an inotify watcher for the
following files: /etc/finit.d/*.conf, /etc/finit.d/available/*.conf,
and /etc/finit.conf.  All file changes between (bootstraps and) calls
to `initctl reload` are tracked, like the mtime backend it replaces.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>

src/conf.c

@@ -25,8 +25,10 @@
 
 #include <dirent.h>
 #include <string.h>
+#include <sys/inotify.h>
 #include <sys/resource.h>
 #include <lite/lite.h>
+#include <lite/queue.h>		/* BSD sys/queue.h API */
 #include <sys/time.h>
 
 #include "finit.h"
@@ -40,8 +42,17 @@
 	(!strncasecmp(l, c, strlen(c)) && (x = (l) + strlen(c)))
 
 struct rlimit global_rlimit[RLIMIT_NLIMITS];
-static int parse_conf(char *file);
 
+struct conf_change {
+	TAILQ_ENTRY(conf_change) link;
+	char *name;
+};
+
+static uev_t w1, w2, w3;
+static TAILQ_HEAD(head, conf_change) conf_change_list = TAILQ_HEAD_INITIALIZER(conf_change_list);
+
+static int parse_conf(char *file);
+static void drop_changes(void);
 
 void conf_parse_cmdline(void)
 {
@@ -308,7 +319,7 @@ static void parse_static(char *line)
 	}
 }
 
-static void parse_dynamic(char *line, struct rlimit rlimit[], struct timeval *mtime)
+static void parse_dynamic(char *line, struct rlimit rlimit[], char *file)
 {
 	char *x;
 	char cmd[CMD_SIZE];
@@ -334,26 +345,26 @@ static void parse_dynamic(char *line, struct rlimit rlimit[], struct timeval *mt
 
 	/* Monitored daemon, will be respawned on exit */
 	if (MATCH_CMD(line, "service ", x)) {
-		service_register(SVC_TYPE_SERVICE, x, rlimit, mtime);
+		service_register(SVC_TYPE_SERVICE, x, rlimit, file);
 		return;
 	}
 
 	/* One-shot task, will not be respawned */
 	if (MATCH_CMD(line, "task ", x)) {
-		service_register(SVC_TYPE_TASK, x, rlimit, mtime);
+		service_register(SVC_TYPE_TASK, x, rlimit, file);
 		return;
 	}
 
 	/* Like task but waits for completion, useful w/ [S] */
 	if (MATCH_CMD(line, "run ", x)) {
-		service_register(SVC_TYPE_RUN, x, rlimit, mtime);
+		service_register(SVC_TYPE_RUN, x, rlimit, file);
 		return;
 	}
 
 	/* Classic inetd service */
 	if (MATCH_CMD(line, "inetd ", x)) {
 #ifdef INETD_ENABLED
-		service_register(SVC_TYPE_INETD, x, rlimit, mtime);
+		service_register(SVC_TYPE_INETD, x, rlimit, file);
 #else
 		_e("Finit built with inetd support disabled, cannot register service inetd %s!", x);
 #endif
@@ -368,7 +379,7 @@ static void parse_dynamic(char *line, struct rlimit rlimit[], struct timeval *mt
 
 	/* Regular or serial TTYs to run getty */
 	if (MATCH_CMD(line, "tty ", x)) {
-		tty_register(strip_line(x), rlimit, mtime);
+		tty_register(strip_line(x), rlimit, file);
 		return;
 	}
 }
@@ -384,7 +395,7 @@ static void tabstospaces(char *line)
 	}
 }
 
-static int parse_conf_dynamic(char *file, struct timeval *mtime)
+static int parse_conf_dynamic(char *file)
 {
 	FILE *fp;
 	struct rlimit rlimit[RLIMIT_NLIMITS];
@@ -409,7 +420,7 @@ static int parse_conf_dynamic(char *file, struct timeval *mtime)
 		tabstospaces(line);
 		_d("%s", line);
 
-		parse_dynamic(line, rlimit, mtime);
+		parse_dynamic(line, rlimit, file);
 	}
 
 	fclose(fp);
@@ -458,21 +469,26 @@ static int parse_conf(char *file)
 	return 0;
 }
 
-/* Reload all *.conf in /etc/finit.d/ */
-int conf_reload_dynamic(void)
+/*
+ * Reload /etc/finit.conf and all *.conf in /etc/finit.d/
+ */
+int conf_reload(void)
 {
 	int i, num;
-	char *dir = rcsd;
 	struct dirent **e;
 
 	/* Mark and sweep */
 	svc_mark_dynamic();
 	tty_mark();
 
-	num = scandir(dir, &e, NULL, alphasort);
+	/* First, read /etc/finit.conf */
+	parse_conf(FINIT_CONF);
+
+	/* Next, read all *.conf in /etc/finit.d/ */
+	num = scandir(rcsd, &e, NULL, alphasort);
 	if (num < 0) {
-		_d("Skipping %s, no files found ...", dir);
-		return 1;
+		_d("Skipping %s, no files found ...", rcsd);
+		return 0;
 	}
 
 	/* Update global limits in case operator used prlimit(1) on us */
@@ -484,13 +500,11 @@ int conf_reload_dynamic(void)
 		char  path[LINE_SIZE];
 		size_t len;
 		struct stat st;
-		struct timeval mtime;
 
-		snprintf(path, sizeof(path), "%s/%s", dir, name);
+		snprintf(path, sizeof(path), "%s/%s", rcsd, name);
 
 		/* Check that it's an actual file ... beyond any symlinks */
 		if (lstat(path, &st)) {
-		lost:
 			_d("Skipping %s, cannot access: %s", path, strerror(errno));
 			continue;
 		}
@@ -521,27 +535,210 @@ int conf_reload_dynamic(void)
 			continue;
 		}
 
-		if (stat(path, &st))
-			goto lost; /* Weird, lost file between here and lstat() */
-
-		TIMESPEC_TO_TIMEVAL(&mtime, &st.st_mtim);
-		parse_conf_dynamic(path, &mtime);
+		parse_conf_dynamic(path);
 	}
 
 	while (num--)
 		free(e[num]);
 	free(e);
 
+	/* Drop record of all .conf changes */
+	drop_changes();
+
+	/*
+	 * Set host name, from %DEFHOST, *.conf or /etc/hostname.  The
+	 * latter wins, if neither exists we default to "noname"
+	 */
 	set_hostname(&hostname);
 
 	return 0;
 }
 
-int conf_parse_config(void)
+static struct conf_change *conf_find(char *file)
+{
+	struct conf_change *node, *tmp;
+
+	TAILQ_FOREACH_SAFE(node, &conf_change_list, link, tmp) {
+		if (string_compare(node->name, file))
+			return node;
+	}
+
+	return NULL;
+}
+
+static void drop_change(struct conf_change *node)
+{
+	if (!node)
+		return;
+
+	TAILQ_REMOVE(&conf_change_list, node, link);
+	free(node->name);
+	free(node);
+}
+
+
+static void drop_changes(void)
+{
+	struct conf_change *node, *tmp;
+
+	TAILQ_FOREACH_SAFE(node, &conf_change_list, link, tmp)
+		drop_change(node);
+}
+
+static int do_change(char *name, uint32_t mask)
+{
+	struct conf_change *node;
+
+	node = conf_find(name);
+	if (mask & (IN_DELETE | IN_MOVED_FROM)) {
+		drop_change(node);
+		return 0;
+	}
+
+	if (node) {
+		_d("Event already registered for %s ...", name);
+		return 0;
+	}
+
+	node = malloc(sizeof(*node));
+	if (!node)
+		return 1;
+
+	node->name = strdup(name);
+	if (!node->name) {
+		free(node);
+		return 1;
+	}
+
+	TAILQ_INSERT_HEAD(&conf_change_list, node,link);
+
+	return 0;
+}
+
+int conf_changed(char *file)
+{
+	char *ptr;
+
+	if (file && (ptr = strrchr(file, '/')))
+		file = ++ptr;
+
+	if (conf_find(file))
+		return 1;
+
+	return 0;
+}
+
+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;
+
+	sz = read(w->fd, ev_buf, sizeof(ev_buf) - 1);
+	if (sz <= 0) {
+		_pe("invalid inotify event");
+		return;
+	}
+	ev_buf[sz] = 0;
+	ev = (struct inotify_event *)ev_buf;
+
+	if (arg) {
+		do_change(arg, ev->mask);
+		return;
+	}
+
+	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");
+			break;
+		}
+	}
+}
+
+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;
+	}
+
+	return 0;
+}
+
+/*
+ * Set up inotify watcher and load all *.conf in /etc/finit.d/
+ */
+int conf_monitor(uev_ctx_t *ctx)
+{
+	int rc = 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_CONF, 0);
+
+	return rc + conf_reload();
+}
+
+/*
+ * Prepare .conf parser and load all .conf files
+ */
+int conf_init(void)
 {
 	hostname = strdup(DEFHOST);
+	w1.fd = w2.fd = w3.fd = -1;
 
-	return parse_conf(FINIT_CONF) || conf_reload_dynamic();
+	return conf_monitor(NULL);
 }
 
 /**

src/conf.h

@@ -31,11 +31,14 @@ extern struct rlimit global_rlimit[];
 int   str2rlim(char *str);
 char *rlim2str(int rlim);
 
+int  conf_init            (void);
+void conf_reload          (void);
+int  conf_changed         (char *file);
+int  conf_monitor         (uev_ctx_t *ctx);
+
 void conf_parse_cmdline   (void);
 int  conf_parse_runlevels (char *runlevels);
 void conf_parse_cond      (svc_t *svc, char *cond);
-int  conf_parse_config    (void);
-void conf_reload_dynamic  (void);
 
 #endif	/* FINIT_CONF_H_ */
 

src/finit.c

@@ -447,19 +447,15 @@ int main(int argc, char* argv[])
 	}
 
 	/*
-	 * Parse /etc/finit.conf and all *.conf in /etc/finit.d/ to
-	 * figure out how to bootstrap the system.
+	 * Initialize .conf system and load static /etc/finit.conf 
 	 */
-	conf_parse_config();
+	conf_init();
 
 	/*
 	 * Start built-in watchdog as soon as possible, if enabled
 	 */
 	wdogpid = watchdog(argv[0]);
 
-	/* Set hostname as soon as possible, for syslog et al. */
-	set_hostname(&hostname);
-
 	/*
 	 * Mount filesystems
 	 */
@@ -488,12 +484,10 @@ int main(int argc, char* argv[])
 	sig_setup(&loop);
 
 	/*
-	 * Reload all *.conf in /etc/finit.d, the mount command (above)
-	 * may have brought in new files (overlayfs).
-	 *
-	 * XXX: Can we detect remounted /etc to trigger this instead?
+	 * Set up inotify watcher for /etc/finit.d and read all .conf
+	 * files to figure out how to bootstrap the system.
 	 */
-	conf_reload_dynamic();
+	conf_monitor(&loop);
 
 	_d("Base FS up, calling hooks ...");
 	plugin_run_hooks(HOOK_BASEFS_UP);

src/service.c

@@ -474,7 +474,7 @@ void service_runlevel(int newlevel)
  * @type:   %SVC_TYPE_SERVICE(0), %SVC_TYPE_TASK(1), %SVC_TYPE_RUN(2)
  * @cfg:    Configuration, complete command, with -- for description text
  * @rlimit: Limits for this service/task/run/inetd, may be global limits
- * @mtime:  The modification time if service is loaded from /etc/finit.d
+ * @file:   The file name service was loaded from
  *
  * This function is used to register commands to be run on different
  * system runlevels with optional username.  The @type argument details
@@ -521,7 +521,7 @@ void service_runlevel(int newlevel)
  * Returns:
  * POSIX OK(0) on success, or non-zero errno exit status on failure.
  */
-int service_register(int type, char *cfg, struct rlimit rlimit[], struct timeval *mtime)
+int service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 {
 	int i = 0;
 	int id = -1;
@@ -727,9 +727,13 @@ recreate:
 	memcpy(svc->rlimit, rlimit, sizeof(svc->rlimit));
 
 	/* New, recently modified or unchanged ... used on reload. */
-	svc_check_dirty(svc, mtime);
-	free(line);
+	if (file && conf_changed(file))
+		svc_mark_dirty(svc);
+	else
+		svc_mark_clean(svc);
 
+	/* Free duped line, from above */
+	free(line);
 	return 0;
 }
 

src/service.h

@@ -28,7 +28,7 @@
 #include "svc.h"
 
 void	  service_runlevel	 (int newlevel);
-int	  service_register	 (int type, char *line, struct rlimit rlimit[], struct timeval *mtime);
+int	  service_register	 (int type, char *line, struct rlimit rlimit[], char *file);
 void      service_unregister     (svc_t *svc);
 
 void      service_runtask_clean  (void);

src/sm.c

@@ -144,7 +144,7 @@ restart:
 
 		/* Make sure to (re)load all *.conf in /etc/finit.d/ */
 		if (runlevel != 0 && runlevel != 6)
-			conf_reload_dynamic();
+			conf_reload();
 
 		/* Reset once flag of runtasks */
 		service_runtask_clean();
@@ -199,7 +199,7 @@ restart:
 
 	case SM_RELOAD_CHANGE_STATE:
 		/* First reload all *.conf in /etc/finit.d/ */
-		conf_reload_dynamic();
+		conf_reload();
 
 		/*
 		 * Then, mark all affected service conditions as in-flux and

src/svc.c

@@ -360,17 +360,6 @@ void svc_mark_clean(svc_t *svc)
 	*((int *)&svc->dirty) = 0;
 }
 
-void svc_check_dirty(svc_t *svc, struct timeval *mtime)
-{
-	if (mtime && timercmp(&svc->mtime, mtime, !=))
-		svc_mark_dirty(svc);
-	else
-		svc_mark_clean(svc);
-
-	svc->mtime.tv_sec = mtime ? mtime->tv_sec : 0;
-	svc->mtime.tv_usec = mtime ? mtime->tv_usec : 0;
-}
-
 /**
  * svc_clean_dynamic - Stop and cleanup stale services removed from /etc/finit.d
  * @cb: Callback to run for each stale service

src/svc.h

@@ -92,9 +92,7 @@ typedef struct svc {
 	pid_t	       pid;
 	const svc_state_t state;       /* Paused, Reloading, Restart, Running, ... */
 	svc_type_t     type;	       /* Service, run, task, inetd, ... */
-	struct timeval mtime;          /* Modification time for .conf from /etc/finit.d/ */
-	const int      dirty;	       /* Set if old mtime != new mtime  => reloaded,
-					* or -1 when marked for removal */
+	const int      dirty;	       /* -1: removal, 0: unmodified, 1: modified */
 	int            starting;       /* ... waiting for pidfile to be re-asserted */
 	int	       runlevels;
 	int            sighup;	       /* This service supports SIGHUP :) */
@@ -148,7 +146,6 @@ void        svc_foreach_type       (int types, void (*cb)(svc_t *));
 svc_t	   *svc_stop_completed	   (void);
 
 void	    svc_mark_dynamic       (void);
-void	    svc_check_dirty        (svc_t *svc, struct timeval *mtime);
 void	    svc_mark_dirty         (svc_t *svc);
 void	    svc_mark_clean         (svc_t *svc);
 void	    svc_clean_dynamic      (void (*cb)(svc_t *));
@@ -168,7 +165,6 @@ static inline void svc_starting    (svc_t *svc) { svc->starting = 1;         }
 static inline void svc_started     (svc_t *svc) { svc->starting = 0;         }
 static inline int  svc_is_starting (svc_t *svc) { return 0 != svc->starting; }
 
-static inline int svc_is_dynamic   (svc_t *svc) { return svc &&  0 != svc->mtime.tv_sec; }
 static inline int svc_is_removed   (svc_t *svc) { return svc && -1 == svc->dirty; }
 static inline int svc_is_changed   (svc_t *svc) { return svc &&  0 != svc->dirty; }
 static inline int svc_is_updated   (svc_t *svc) { return svc &&  1 == svc->dirty; }

src/tty.c

@@ -90,22 +90,8 @@ void tty_mark(void)
 {
 	tty_node_t *tty;
 
-	LIST_FOREACH(tty, &tty_list, link) {
-		if (tty->mtime.tv_sec)
-			tty->dirty = -1;
-	}
-}
-
-void tty_check(tty_node_t *tty, struct timeval *mtime)
-{
-	if (mtime && timercmp(&tty->mtime, mtime, !=))
-		tty->dirty = 1;	/* Modified, restart */
-	else
-		tty->dirty = 0;	/* Not modified */
-
-	/* Update mtime, if given */
-	tty->mtime.tv_sec  = mtime ? mtime->tv_sec  : 0;
-	tty->mtime.tv_usec = mtime ? mtime->tv_usec : 0;
+	LIST_FOREACH(tty, &tty_list, link)
+		tty->dirty = -1;
 }
 
 void tty_sweep(void)
@@ -113,14 +99,15 @@ void tty_sweep(void)
 	tty_node_t *tty, *tmp;
 
 	LIST_FOREACH_SAFE(tty, &tty_list, link, tmp) {
-		if (tty->mtime.tv_sec && tty->dirty) {
-			_d("TTY %s dirty, stopping ...", tty->data.name);
-			tty_stop(&tty->data);
+		if (!tty->dirty)
+			continue;
 
-			if (tty->dirty == -1) {
-				_d("TTY %s removed, cleaning up.", tty->data.name);
-				tty_unregister(tty);
-			}
+		_d("TTY %s dirty, stopping ...", tty->data.name);
+		tty_stop(&tty->data);
+
+		if (tty->dirty == -1) {
+			_d("TTY %s removed, cleaning up.", tty->data.name);
+			tty_unregister(tty);
 		}
 	}
 }
@@ -129,7 +116,7 @@ void tty_sweep(void)
  * tty_register - Register a getty on a device
  * @line:   Configuration, text after initial "tty"
  * @rlimit: Limits for this service/task/run/inetd, may be global limits
- * @mtime:  Modification time, to propagate to lower layers
+ * @file:   The file name TTY was loaded from
  *
  * A Finit tty line can use the internal getty implementation or an
  * external one, like the BusyBox getty for instance.  This function
@@ -147,7 +134,7 @@ void tty_sweep(void)
  * Different getty implementations prefer the TTY device argument in
  * different order, so take care to investigate this first.
  */
-int tty_register(char *line, struct rlimit rlimit[], struct timeval *mtime)
+int tty_register(char *line, struct rlimit rlimit[], char *file)
 {
 	tty_node_t *entry;
 	int         insert = 0, noclear = 0, nowait = 0;
@@ -282,7 +269,10 @@ int tty_register(char *line, struct rlimit rlimit[], struct timeval *mtime)
 	/* Register configured limits */
 	memcpy(entry->data.rlimit, rlimit, sizeof(entry->data.rlimit));
 
-	tty_check(entry, mtime);
+	if (file && conf_changed(file))
+		entry->dirty = 1; /* Modified, restart */
+	else
+		entry->dirty = 0; /* Not modified */
 	_d("TTY %s is %sdirty", dev, entry->dirty ? "" : "NOT ");
 
 	return 0;

src/tty.h

@@ -54,7 +54,6 @@ typedef struct tty_node {
 	finit_tty_t data;
 
 	/* XXX: Yes, TTYs should be refactored into a separate SVC type. */
-	struct timeval mtime;          /* Modification time for .conf from /etc/finit.d/ */
 	int            dirty;	       /* Set if old mtime != new mtime  => reloaded,
 					* or -1 when marked for removal */
 } tty_node_t;
@@ -62,10 +61,9 @@ typedef struct tty_node {
 //extern LIST_HEAD(, tty_node) tty_list;
 
 void	    tty_mark	    (void);
-void	    tty_check	    (tty_node_t *tty, struct timeval *mtime);
 void	    tty_sweep	    (void);
 
-int	    tty_register    (char *line, struct rlimit rlimit[], struct timeval *mtime);
+int	    tty_register    (char *line, struct rlimit rlimit[], char *file);
 int	    tty_unregister  (tty_node_t *tty);
 
 tty_node_t *tty_find	    (char *dev);