lightningd: simplify --daemon.

Dumb programs which have a --daemon option call fork() early.  This is
terrible UX since startup errors get lost: the program exits with
"success" immediately then you discover via the logs that it didn't
start at all.

However, forking late introduced a heap of problems with changing
pids.  Instead, fork early but keep stderr and the parent around: if
we fail early on, the parent fails with us.  We release our parent
with an explicit action just before the main loop.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Makefile

@@ -70,7 +70,6 @@ CCAN_OBJS :=					\
 	ccan-crypto-sha256.o			\
 	ccan-crypto-shachain.o			\
 	ccan-crypto-siphash24.o			\
-	ccan-daemonize.o			\
 	ccan-err.o				\
 	ccan-fdpass.o				\
 	ccan-htable.o				\
@@ -132,7 +131,6 @@ CCAN_HEADERS :=						\
 	$(CCANDIR)/ccan/crypto/sha256/sha256.h		\
 	$(CCANDIR)/ccan/crypto/shachain/shachain.h	\
 	$(CCANDIR)/ccan/crypto/siphash24/siphash24.h	\
-	$(CCANDIR)/ccan/daemonize/daemonize.h		\
 	$(CCANDIR)/ccan/endian/endian.h			\
 	$(CCANDIR)/ccan/err/err.h			\
 	$(CCANDIR)/ccan/fdpass/fdpass.h			\
@@ -617,8 +615,6 @@ ccan-opt-parse.o: $(CCANDIR)/ccan/opt/parse.c
 	$(CC) $(CFLAGS) -c -o $@ $<
 ccan-opt-usage.o: $(CCANDIR)/ccan/opt/usage.c
 	$(CC) $(CFLAGS) -c -o $@ $<
-ccan-daemonize.o: $(CCANDIR)/ccan/daemonize/daemonize.c
-	$(CC) $(CFLAGS) -c -o $@ $<
 ccan-err.o: $(CCANDIR)/ccan/err/err.c
 	$(CC) $(CFLAGS) -c -o $@ $<
 ccan-noerr.o: $(CCANDIR)/ccan/noerr/noerr.c

ccan/ccan/daemonize/LICENSE

@@ -1 +0,0 @@
-../../licenses/BSD-MIT

ccan/ccan/daemonize/_info

@@ -1,54 +0,0 @@
-#include "config.h"
-#include <stdio.h>
-#include <string.h>
-
-/**
- * daemonize - routine to turn a process into a well-behaved daemon.
- *
- * Daemons should detach themselves thoroughly from the process which launched
- * them, and not prevent any filesystems from being unmounted.  daemonize()
- * helps with the process.
- *
- * Example:
- *	#include <ccan/daemonize/daemonize.h>
- *	#include <ccan/str/str.h>
- *	#include <err.h>
- *	#include <unistd.h>
- *	#include <stdlib.h>
- *	
- *	static void usage(const char *name)
- *	{
- *		errx(1, "Usage: %s [--daemonize]\n", name);
- *	}
- *	
- *	// Wait for a minute, possibly as a daemon.
- *	int main(int argc, char *argv[])
- *	{
- *		if (argc != 1) {
- *			if (argc == 2 && streq(argv[1], "--daemonize")) {
- *				if (!daemonize())
- *					err(1, "Failed to become daemon");
- *			} else
- *				usage(argv[1]);
- *		}
- *		sleep(60);
- *		exit(0);
- *	}
- *
- * License: BSD-MIT
- */
-int main(int argc, char *argv[])
-{
-	if (argc != 2)
-		return 1;
-
-	if (strcmp(argv[1], "depends") == 0) {
-		return 0;
-	}
-
-	if (strcmp(argv[1], "libs") == 0) {
-		return 0;
-	}
-
-	return 1;
-}

ccan/ccan/daemonize/daemonize.c

@@ -1,45 +0,0 @@
-/* Licensed under BSD-MIT - see LICENSE file for details */
-#include <ccan/daemonize/daemonize.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-
-/* This code is based on Stevens Advanced Programming in the UNIX
- * Environment. */
-bool daemonize(void)
-{
-	pid_t pid;
-
-	/* Separate from our parent via fork, so init inherits us. */
-	if ((pid = fork()) < 0)
-		return false;
-	/* use _exit() to avoid triggering atexit() processing */
-	if (pid != 0)
-		_exit(0);
-
-	/* Don't hold files open. */
-	close(STDIN_FILENO);
-	close(STDOUT_FILENO);
-	close(STDERR_FILENO);
-
-	/* Many routines write to stderr; that can cause chaos if used
-	 * for something else, so set it here. */
-	if (open("/dev/null", O_WRONLY) != 0)
-		return false;
-	if (dup2(0, STDERR_FILENO) != STDERR_FILENO)
-		return false;
-	close(0);
-
-	/* Session leader so ^C doesn't whack us. */
-	if (setsid() == (pid_t)-1)
-	        return false;
-	/* Move off any mount points we might be in. */
-	if (chdir("/") != 0)
-		return false;
-
-	/* Discard our parent's old-fashioned umask prejudices. */
-	umask(0);
-	return true;
-}

ccan/ccan/daemonize/daemonize.h

@@ -1,21 +0,0 @@
-/* Licensed under BSD-MIT - see LICENSE file for details */
-#ifndef CCAN_DAEMONIZE_H
-#define CCAN_DAEMONIZE_H
-#include <stdbool.h>
-
-/**
- * daemonize - turn this process into a daemon.
- *
- * This routine forks us off to become a daemon.  It returns false on failure
- * (eg. fork(), chdir or open failed) and sets errno.
- *
- * Side effects for programmers to be aware of:
- *  - PID changes (our parent exits, we become child of init)
- *  - stdin and stdout file descriptors are closed
- *  - stderr is reopened to /dev/null so you don't reuse it
- *  - Current working directory changes to /
- *  - Umask is set to 0.
- */
-bool daemonize(void);
-
-#endif /* CCAN_DAEMONIZE_H */

ccan/ccan/daemonize/test/run.c

@@ -1,76 +0,0 @@
-#include <ccan/daemonize/daemonize.h>
-#include <ccan/daemonize/daemonize.c>
-#include <ccan/tap/tap.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <err.h>
-#include <errno.h>
-#include <string.h>
-
-struct child_data {
-	pid_t pid;
-	pid_t ppid;
-	bool in_root_dir;
-	int read_from_stdin, write_to_stdout, write_to_stderr;
-};
-
-int main(int argc, char *argv[])
-{
-	int fds[2];
-	struct child_data daemonized;
-	pid_t pid;
-
-	plan_tests(5);
-
-	if (pipe(fds) != 0)
-		err(1, "Failed pipe");
-
-	/* Since daemonize forks and parent exits, we need to fork
-	 * that parent. */
-	pid = fork();
-	if (pid == -1)
-		err(1, "Failed fork");
-
-	if (pid == 0) {
-		char buffer[2];
-		pid = getpid();
-		daemonize();
-		/* Keep valgrind happy about uninitialized bytes. */
-		memset(&daemonized, 0, sizeof(daemonized));
-		daemonized.pid = getpid();
-		daemonized.in_root_dir = (getcwd(buffer, 2) != NULL);
-		daemonized.read_from_stdin
-			= read(STDIN_FILENO, buffer, 1) == -1 ? errno : 0;
-		daemonized.write_to_stdout
-			= write(STDOUT_FILENO, buffer, 1) == -1 ? errno : 0;
-		if (write(STDERR_FILENO, buffer, 1) != 1) {
-			daemonized.write_to_stderr = errno;
-			if (daemonized.write_to_stderr == 0)
-				daemonized.write_to_stderr = -1;
-		} else
-			daemonized.write_to_stderr = 0;
-
-		/* Make sure parent exits. */
-		while (getppid() == pid)
-			sleep(1);
-		daemonized.ppid = getppid();
-		if (write(fds[1], &daemonized, sizeof(daemonized))
-		    != sizeof(daemonized))
-			exit(1);
-		exit(0);
-	}
-
-	if (read(fds[0], &daemonized, sizeof(daemonized)) != sizeof(daemonized))
-		err(1, "Failed read");
-
-	ok1(daemonized.pid != pid);
-#if 0	/* Believe it or not, this fails under Ubuntu 13.10 (Upstart) */
-	ok1(daemonized.ppid == 1);
-#endif
-	ok1(daemonized.in_root_dir);
-	ok1(daemonized.read_from_stdin == EBADF);
-	ok1(daemonized.write_to_stdout == EBADF);
-	ok1(daemonized.write_to_stderr == 0);
-
-	return exit_status();
-}

lightningd/lightningd.c

@@ -42,7 +42,6 @@
 #include <ccan/array_size/array_size.h>
 #include <ccan/cast/cast.h>
 #include <ccan/crypto/hkdf_sha256/hkdf_sha256.h>
-#include <ccan/daemonize/daemonize.h>
 #include <ccan/err/err.h>
 #include <ccan/io/fdpass/fdpass.h>
 #include <ccan/io/io.h>
@@ -77,6 +76,7 @@
 #include <lightningd/options.h>
 #include <onchaind/onchain_wire.h>
 #include <signal.h>
+#include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 
@@ -198,7 +198,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx)
 
 	/*~ This is detailed in chaintopology.c */
 	ld->topology = new_topology(ld, ld->log);
-	ld->daemon = false;
+	ld->daemon_parent_fd = -1;
 	ld->config_filename = NULL;
 	ld->pidfile = NULL;
 	ld->proxyaddr = NULL;
@@ -501,30 +501,37 @@ static void init_txfilter(struct wallet *w, struct txfilter *filter)
  * don't prevent unmounting whatever filesystem you happen to start in.
  *
  * But we define every path relative to our (~/.lightning) data dir, so we
- * make sure we stay there.
+ * make sure we stay there.  The rest of this is taken from ccan/daemonize,
+ * which was based on W. Richard Steven's advice in Programming in The Unix
+ * Environment.
  */
-static void daemonize_but_keep_dir(struct lightningd *ld)
+static void complete_daemonize(struct lightningd *ld)
 {
-	/* daemonize moves us into /, but we want to be here */
-	const char *cwd = path_cwd(NULL);
+	int ok_status = 0;
 
-	/*~ SQLite3 does NOT like being open across fork(), a.k.a. daemonize() */
-	db_close_for_fork(ld->wallet->db);
-	if (!cwd)
-		fatal("Could not get current directory: %s", strerror(errno));
-	if (!daemonize())
-		fatal("Could not become a daemon: %s", strerror(errno));
+	/* Don't hold files open. */
+	close(STDIN_FILENO);
+	close(STDOUT_FILENO);
+	close(STDERR_FILENO);
 
-	/*~ Move back: important, since lightning dir may be relative! */
-	if (chdir(cwd) != 0)
-		fatal("Could not return to directory %s: %s",
-		      cwd, strerror(errno));
+	/* Many routines write to stderr; that can cause chaos if used
+	 * for something else, so set it here. */
+	if (open("/dev/null", O_WRONLY) != 0)
+		fatal("Could not open /dev/null: %s", strerror(errno));
+	if (dup2(0, STDERR_FILENO) != STDERR_FILENO)
+		fatal("Could not dup /dev/null for stderr: %s", strerror(errno));
+	close(0);
 
-	db_reopen_after_fork(ld->wallet->db);
+	/* Session leader so ^C doesn't whack us. */
+	if (setsid() == (pid_t)-1)
+		fatal("Could not setsid: %s", strerror(errno));
 
-	/*~ Why not allocate cwd off tmpctx?  Probably because this code predates
-	 * tmpctx.  So we free manually here. */
-	tal_free(cwd);
+	/* Discard our parent's old-fashioned umask prejudices. */
+	umask(0);
+
+	/* OK, parent, you can exit(0) now. */
+	write_all(ld->daemon_parent_fd, &ok_status, sizeof(ok_status));
+	close(ld->daemon_parent_fd);
 }
 
 /*~ It's pretty standard behaviour (especially for daemons) to create and
@@ -785,13 +792,6 @@ int main(int argc, char *argv[])
 	 * in case that runs into trouble. */
 	crashlog = ld->log;
 
-	/*~ We defer --daemon until we've completed most initialization: that
-	 *  way we'll exit with an error rather than silently exiting 0, then
-	 *  realizing we can't start and forcing the confused user to read the
-	 *  logs. */
-	if (ld->daemon)
-		daemonize_but_keep_dir(ld);
-
 	/*~ We have to do this after daemonize, since that changes our pid! */
 	pidfile_write(ld, pid_fd);
 
@@ -830,6 +830,16 @@ int main(int argc, char *argv[])
 	 *  can start the poll loop which queries bitcoind for new blocks. */
 	begin_topology(ld->topology);
 
+	/*~ To handle --daemon, we fork the daemon early (otherwise we hit
+	 * issues with our pid changing), but keep the parent around until
+	 * we've completed most initialization: that way we'll exit with an
+	 * error rather than silently exiting 0, then realizing we can't start
+	 * and forcing the confused user to read the logs.
+	 *
+	 * But we're all initialized, so detach and have parent exit now. */
+	if (ld->daemon_parent_fd != -1)
+		complete_daemonize(ld);
+
 	/*~ The root of every backtrace (almost).  This is our main event
 	 *  loop. */
 	void *io_loop_ret = io_loop_with_timers(ld);

lightningd/lightningd.h

@@ -76,8 +76,9 @@ struct lightningd {
 	/* The directory to find all the subdaemons. */
 	const char *daemon_dir;
 
-	/* Are we told to run in the background. */
-	bool daemon;
+	/* If we told to run in the background, this is our parent fd, otherwise
+	 * -1. */
+	int daemon_parent_fd;
 
 	int pid_fd;
 

lightningd/options.c

@@ -36,6 +36,7 @@
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wire/wire.h>
 
@@ -721,6 +722,41 @@ static char *opt_lightningd_usage(struct lightningd *ld)
 	return NULL;
 }
 
+static char *opt_start_daemon(struct lightningd *ld)
+{
+	int fds[2];
+	int exitcode, pid;
+
+	/* Already a daemon?  OK. */
+	if (ld->daemon_parent_fd != -1)
+		return NULL;
+
+	if (pipe(fds) != 0)
+		err(1, "Creating pipe to talk to --daemon");
+
+	pid = fork();
+	if (pid == -1)
+		err(1, "Fork failed for --daemon");
+
+	if (pid == 0) {
+		/* Child returns, continues as normal. */
+		close(fds[0]);
+		ld->daemon_parent_fd = fds[1];
+		return NULL;
+	}
+
+	/* OK, we are the parent.  We exit with status told to us by
+	 * child. */
+	close(fds[1]);
+	if (read(fds[0], &exitcode, sizeof(exitcode)) == sizeof(exitcode))
+		_exit(exitcode);
+	/* It died before writing exitcode (presumably 0), so we grab it */
+	waitpid(pid, &exitcode, 0);
+	if (WIFEXITED(exitcode))
+		_exit(WEXITSTATUS(exitcode));
+	errx(1, "Died with signal %u", WTERMSIG(exitcode));
+}
+
 static char *opt_ignore_talstr(const char *arg, char **p)
 {
 	return NULL;
@@ -809,6 +845,10 @@ static void register_opts(struct lightningd *ld)
 			       opt_set_bool_arg, opt_show_bool,
 			       &ld->use_proxy_always, "Use the proxy always");
 
+	/* This immediately makes is a daemon. */
+	opt_register_early_noarg("--daemon", opt_start_daemon, ld,
+				 "Run in the background, suppress stdout/stderr");
+
 	opt_register_arg("--rpc-file", opt_set_talstr, opt_show_charp,
 			 &ld->rpc_filename,
 			 "Set JSON-RPC socket (or /dev/tty)");
@@ -847,8 +887,6 @@ static void register_opts(struct lightningd *ld)
 			 &ld->pidfile,
 			 "Specify pid file");
 
-	opt_register_noarg("--daemon", opt_set_bool, &ld->daemon,
-			 "Run in the background, suppress stdout/stderr");
 	opt_register_arg("--ignore-fee-limits", opt_set_bool_arg, opt_show_bool,
 			 &ld->config.ignore_fee_limits,
 			 "(DANGEROUS) allow peer to set any feerate");
@@ -1099,6 +1137,10 @@ static void add_config(struct lightningd *ld,
 			answer = tal_fmt(name0, "%s",
 					 (!ld->reconnect && !ld->listen)
 					 ? "true" : "false");
+		} else if (opt->cb == (void *)opt_start_daemon) {
+			answer = tal_fmt(name0, "%s",
+					 ld->daemon_parent_fd == -1
+					 ? "false" : "true");
 		} else {
 			/* Insert more decodes here! */
 			assert(!"A noarg option was added but was not handled");

lightningd/test/run-find_my_abspath.c

@@ -51,18 +51,12 @@ void db_assert_no_outstanding_statements(void)
 /* Generated stub for db_begin_transaction_ */
 void db_begin_transaction_(struct db *db UNNEEDED, const char *location UNNEEDED)
 { fprintf(stderr, "db_begin_transaction_ called!\n"); abort(); }
-/* Generated stub for db_close_for_fork */
-void db_close_for_fork(struct db *db UNNEEDED)
-{ fprintf(stderr, "db_close_for_fork called!\n"); abort(); }
 /* Generated stub for db_commit_transaction */
 void db_commit_transaction(struct db *db UNNEEDED)
 { fprintf(stderr, "db_commit_transaction called!\n"); abort(); }
 /* Generated stub for db_get_intvar */
 s64 db_get_intvar(struct db *db UNNEEDED, char *varname UNNEEDED, s64 defval UNNEEDED)
 { fprintf(stderr, "db_get_intvar called!\n"); abort(); }
-/* Generated stub for db_reopen_after_fork */
-void db_reopen_after_fork(struct db *db UNNEEDED)
-{ fprintf(stderr, "db_reopen_after_fork called!\n"); abort(); }
 /* Generated stub for fatal */
 void   fatal(const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "fatal called!\n"); abort(); }

tests/test_misc.py

@@ -659,7 +659,6 @@ def test_cli(node_factory):
     assert only_one(j['invoices'])['label'] == 'l"[]{}'
 
 
-@pytest.mark.xfail(strict=True)
 def test_daemon_option(node_factory):
     """
     Make sure --daemon at least vaguely works!

wallet/db.c

@@ -778,29 +778,6 @@ struct db *db_setup(const tal_t *ctx, struct lightningd *ld, struct log *log)
 	return db;
 }
 
-void db_close_for_fork(struct db *db)
-{
-	/* https://www.sqlite.org/faq.html#q6
-	 *
-	 * Under Unix, you should not carry an open SQLite database across a
-	 * fork() system call into the child process. */
-	if (sqlite3_close(db->sql) != SQLITE_OK)
-		db_fatal("sqlite3_close: %s", sqlite3_errmsg(db->sql));
-	db->sql = NULL;
-}
-
-void db_reopen_after_fork(struct db *db)
-{
-	int err = sqlite3_open_v2(db->filename, &db->sql,
-				  SQLITE_OPEN_READWRITE, NULL);
-
-	if (err != SQLITE_OK) {
-		db_fatal("failed to re-open database %s: %s", db->filename,
-			 sqlite3_errstr(err));
-	}
-	setup_open_db(db);
-}
-
 s64 db_get_intvar(struct db *db, char *varname, s64 defval)
 {
 	s64 res;

wallet/db.h

@@ -149,10 +149,6 @@ void db_stmt_done(sqlite3_stmt *stmt);
 /* Call when you know there should be no outstanding db statements. */
 void db_assert_no_outstanding_statements(void);
 
-/* Do not keep db open across a fork: needed for --daemon */
-void db_close_for_fork(struct db *db);
-void db_reopen_after_fork(struct db *db);
-
 #define sqlite3_column_arr(ctx, stmt, col, type)			\
 	((type *)sqlite3_column_arr_((ctx), (stmt), (col),		\
 				     sizeof(type), TAL_LABEL(type, "[]"), \