lightningd: minor cleanups

Code changes:
1. Expose daemon_poll() so lightningd can call it directly, which avoids us
   having store a global and document it.
2. Remove the (undocumented, unused, forgotten) --rpc-file="" option to disable
   JSON RPC.
3. Move the ickiness of finding the executable path into subd.c, so it doesn't
   distract from lightningd.c overview.

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

Author: rustyrussell

common/daemon.c

@@ -7,7 +7,6 @@
 #include <common/status.h>
 #include <common/utils.h>
 #include <common/version.h>
-#include <poll.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -66,7 +65,7 @@ static void crashlog_activate(void)
 }
 #endif
 
-static int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout)
+int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout)
 {
 	const char *t;
 

common/daemon.h

@@ -1,12 +1,16 @@
 #ifndef LIGHTNING_COMMON_DAEMON_H
 #define LIGHTNING_COMMON_DAEMON_H
 #include "config.h"
+#include <poll.h>
 
 /* Common setup for all daemons */
 void daemon_setup(const char *argv0,
 		  void (*backtrace_print)(const char *fmt, ...),
 		  void (*backtrace_exit)(void));
 
+/* Exposed for lightningd's use. */
+int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout);
+
 /* Shutdown for a valgrind-clean exit (frees everything) */
 void daemon_shutdown(void);
 

lightningd/jsonrpc.c

@@ -600,9 +600,6 @@ void setup_jsonrpc(struct lightningd *ld, const char *rpc_filename)
 	struct sockaddr_un addr;
 	int fd, old_umask;
 
-	if (streq(rpc_filename, ""))
-		return;
-
 	if (streq(rpc_filename, "/dev/tty")) {
 		fd = open(rpc_filename, O_RDWR);
 		if (fd == -1)

lightningd/lightningd.c

@@ -232,62 +232,24 @@ static bool has_all_subdaemons(const char* daemon_dir)
 	return !missing_daemon;
 }
 
-/* This routine tries to determine what path the lightningd binary is in.
- * It's not actually that simple! */
-static const char *find_my_path(const tal_t *ctx, const char *argv0)
+/* Returns the directory this executable is running from */
+static const char *find_my_directory(const tal_t *ctx, const char *argv0)
 {
-	char *me;
-
-	/* A command containing / is run relative to the current directory,
-	 * not searched through the path.  The shell sets argv0 to the command
-	 * run, though something else could set it to a arbitrary value and
-	 * this logic would be wrong. */
-	if (strchr(argv0, PATH_SEP)) {
-		const char *path;
-		/* Absolute paths are easy. */
-		if (strstarts(argv0, PATH_SEP_STR))
-			path = argv0;
-		/* It contains a '/', it's relative to current dir. */
-		else
-			path = path_join(tmpctx, path_cwd(tmpctx), argv0);
-
-		me = path_canon(ctx, path);
-		if (!me || access(me, X_OK) != 0)
-			errx(1, "I cannot find myself at %s based on my name %s",
-			     path, argv0);
-	} else {
-		/* No /, search path */
-		char **pathdirs;
-		const char *pathenv = getenv("PATH");
-		size_t i;
-
-		/* This replicates the standard shell path search algorithm */
-		if (!pathenv)
-			errx(1, "Cannot find myself: no $PATH set");
-
-		pathdirs = tal_strsplit(tmpctx, pathenv, ":", STR_NO_EMPTY);
-		me = NULL;
-		for (i = 0; pathdirs[i]; i++) {
-			/* This returns NULL if it doesn't exist. */
-			me = path_canon(ctx,
-					path_join(tmpctx, pathdirs[i], argv0));
-			if (me && access(me, X_OK) == 0)
-				break;
-			/* Nope, try again. */
-			me = tal_free(me);
-		}
-		if (!me)
-			errx(1, "Cannot find %s in $PATH", argv0);
-	}
+	/* find_my_abspath simply exits on failure, so never returns NULL. */
+	const char *me = find_my_abspath(NULL, argv0);
 
 	/*~ The caller just wants the directory we're in.
 	 *
-	 * Note the magic "take()" macro here: it annotates a pointer as "to
+	 * Note the magic `take()` macro here: it annotates a pointer as "to
 	 * be taken", and the recipient is expected to take ownership of the
-	 * pointer.
+	 * pointer.  This improves efficiency because the recipient might
+	 * choose to use or even keep it rather than make a copy (or it
+	 * might just free it).
 	 *
-	 * Many CCAN and our own routines support this, but if you hand a take()
-	 * to a non-take routine unfortunately you don't get a compile error.
+	 * Many CCAN and our own routines support this, but if you hand a
+	 * `take()` to a routine which *doesn't* expect it, unfortunately you
+	 * don't get a compile error (we have runtime detection for this
+	 * case, however).
 	 */
 	return path_dirname(ctx, take(me));
 }
@@ -310,7 +272,7 @@ static const char *find_my_pkglibexec_path(const tal_t *ctx,
 /* Determine the correct daemon dir. */
 static const char *find_daemon_dir(const tal_t *ctx, const char *argv0)
 {
-	const char *my_path = find_my_path(ctx, argv0);
+	const char *my_path = find_my_directory(ctx, argv0);
 	/* If we're running in-tree, all the subdaemons are with lightningd. */
 	if (has_all_subdaemons(my_path))
 		return my_path;
@@ -470,19 +432,17 @@ static void pidfile_create(const struct lightningd *ld)
 	/* Leave file open: we close it implicitly when we exit */
 }
 
-/*~ Yuck, we need a global here.
- *
- * ccan/io allows overriding the poll() function for special effects: for
- * lightningd, we make sure we haven't left a db transaction open.  All
- * daemons which use ccan/io add sanity checks in this loop, so we chain
- * that after our own override.
- */
-static int (*io_poll_debug)(struct pollfd *, nfds_t, int);
+/*~ ccan/io allows overriding the poll() function that is the very core
+ * of the event loop it runs for us.  We override it so that we can do
+ * extra sanity checks, and it's also a good point to free the tmpctx. */
 static int io_poll_lightningd(struct pollfd *fds, nfds_t nfds, int timeout)
 {
+	/*~ In particular, we should *not* have left a database transaction
+	 * open! */
 	db_assert_no_outstanding_statements();
 
-	return io_poll_debug(fds, nfds, timeout);
+	/* The other checks and freeing tmpctx are common to all daemons. */
+	return daemon_poll(fds, nfds, timeout);
 }
 
 /*~ Ever had one of those functions which doesn't quite fit anywhere?  Me too.
@@ -540,7 +500,7 @@ int main(int argc, char *argv[])
 	ld->owned_txfilter = txfilter_new(ld);
 
 	/*~ This is the ccan/io central poll override from above. */
-	io_poll_debug = io_poll_override(io_poll_lightningd);
+	io_poll_override(io_poll_lightningd);
 
 	/*~ Set up HSM: it knows our node secret key, so tells us who we are. */
 	hsm_init(ld);

lightningd/subd.c

@@ -822,3 +822,53 @@ bool dev_disconnect_permanent(struct lightningd *ld)
 	return false;
 }
 #endif /* DEVELOPER */
+
+/* Ugly helper to get full pathname of the current binary. */
+const char *find_my_abspath(const tal_t *ctx, const char *argv0)
+{
+	char *me;
+
+	/* A command containing / is run relative to the current directory,
+	 * not searched through the path.  The shell sets argv0 to the command
+	 * run, though something else could set it to a arbitrary value and
+	 * this logic would be wrong. */
+	if (strchr(argv0, PATH_SEP)) {
+		const char *path;
+		/* Absolute paths are easy. */
+		if (strstarts(argv0, PATH_SEP_STR))
+			path = argv0;
+		/* It contains a '/', it's relative to current dir. */
+		else
+			path = path_join(tmpctx, path_cwd(tmpctx), argv0);
+
+		me = path_canon(ctx, path);
+		if (!me || access(me, X_OK) != 0)
+			errx(1, "I cannot find myself at %s based on my name %s",
+			     path, argv0);
+	} else {
+		/* No /, search path */
+		char **pathdirs;
+		const char *pathenv = getenv("PATH");
+		size_t i;
+
+		/* This replicates the standard shell path search algorithm */
+		if (!pathenv)
+			errx(1, "Cannot find myself: no $PATH set");
+
+		pathdirs = tal_strsplit(tmpctx, pathenv, ":", STR_NO_EMPTY);
+		me = NULL;
+		for (i = 0; pathdirs[i]; i++) {
+			/* This returns NULL if it doesn't exist. */
+			me = path_canon(ctx,
+					path_join(tmpctx, pathdirs[i], argv0));
+			if (me && access(me, X_OK) == 0)
+				break;
+			/* Nope, try again. */
+			me = tal_free(me);
+		}
+		if (!me)
+			errx(1, "Cannot find %s in $PATH", argv0);
+	}
+
+	return me;
+}

lightningd/subd.h

@@ -202,6 +202,9 @@ void subd_release_channel(struct subd *owner, void *channel);
  */
 void subd_shutdown(struct subd *subd, unsigned int seconds);
 
+/* Ugly helper to get full pathname of the current binary. */
+const char *find_my_abspath(const tal_t *ctx, const char *argv0);
+
 #if DEVELOPER
 char *opt_subd_debug(const char *optarg, struct lightningd *ld);
 char *opt_subd_dev_disconnect(const char *optarg, struct lightningd *ld);

lightningd/test/run-find_my_path.c renamed to lightningd/test/run-find_my_abspath.c

@@ -3,6 +3,7 @@ int unused_main(int argc, char *argv[]);
 #include "../../common/base32.c"
 #include "../../common/wireaddr.c"
 #include "../lightningd.c"
+#include "../subd.c"
 
 /* AUTOGENERATED MOCKS START */
 /* Generated stub for activate_peers */
@@ -21,6 +22,9 @@ void connectd_activate(struct lightningd *ld UNNEEDED)
 /* Generated stub for connectd_init */
 int connectd_init(struct lightningd *ld UNNEEDED)
 { fprintf(stderr, "connectd_init called!\n"); abort(); }
+/* Generated stub for daemon_poll */
+int daemon_poll(struct pollfd *fds UNNEEDED, nfds_t nfds UNNEEDED, int timeout UNNEEDED)
+{ fprintf(stderr, "daemon_poll called!\n"); abort(); }
 /* Generated stub for daemon_setup */
 void daemon_setup(const char *argv0 UNNEEDED,
 		  void (*backtrace_print)(const char *fmt UNNEEDED, ...) UNNEEDED,
@@ -53,6 +57,18 @@ void   fatal(const char *fmt UNNEEDED, ...)
 /* Generated stub for free_htlcs */
 void free_htlcs(struct lightningd *ld UNNEEDED, const struct channel *channel UNNEEDED)
 { fprintf(stderr, "free_htlcs called!\n"); abort(); }
+/* Generated stub for fromwire_status_fail */
+bool fromwire_status_fail(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, enum status_failreason *failreason UNNEEDED, wirestring **desc UNNEEDED)
+{ fprintf(stderr, "fromwire_status_fail called!\n"); abort(); }
+/* Generated stub for fromwire_status_peer_billboard */
+bool fromwire_status_peer_billboard(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, bool *perm UNNEEDED, wirestring **happenings UNNEEDED)
+{ fprintf(stderr, "fromwire_status_peer_billboard called!\n"); abort(); }
+/* Generated stub for fromwire_status_peer_error */
+bool fromwire_status_peer_error(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct channel_id *channel UNNEEDED, wirestring **desc UNNEEDED, struct crypto_state *crypto_state UNNEEDED, u8 **error_for_them UNNEEDED)
+{ fprintf(stderr, "fromwire_status_peer_error called!\n"); abort(); }
+/* Generated stub for get_log_book */
+struct log_book *get_log_book(const struct log *log UNNEEDED)
+{ fprintf(stderr, "get_log_book called!\n"); abort(); }
 /* Generated stub for gossip_init */
 void gossip_init(struct lightningd *ld UNNEEDED, int connectd_fd UNNEEDED)
 { fprintf(stderr, "gossip_init called!\n"); abort(); }
@@ -84,6 +100,12 @@ void log_backtrace_exit(void)
 /* Generated stub for log_backtrace_print */
 void log_backtrace_print(const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "log_backtrace_print called!\n"); abort(); }
+/* Generated stub for log_prefix */
+const char *log_prefix(const struct log *log UNNEEDED)
+{ fprintf(stderr, "log_prefix called!\n"); abort(); }
+/* Generated stub for log_status_msg */
+bool log_status_msg(struct log *log UNNEEDED, const u8 *msg UNNEEDED)
+{ fprintf(stderr, "log_status_msg called!\n"); abort(); }
 /* Generated stub for new_log */
 struct log *new_log(const tal_t *ctx UNNEEDED, struct log_book *record UNNEEDED, const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "new_log called!\n"); abort(); }
@@ -110,9 +132,6 @@ void setup_jsonrpc(struct lightningd *ld UNNEEDED, const char *rpc_filename UNNE
 void setup_topology(struct chain_topology *topology UNNEEDED, struct timers *timers UNNEEDED,
 		    u32 min_blockheight UNNEEDED, u32 max_blockheight UNNEEDED)
 { fprintf(stderr, "setup_topology called!\n"); abort(); }
-/* Generated stub for subd_shutdown */
-void subd_shutdown(struct subd *subd UNNEEDED, unsigned int seconds UNNEEDED)
-{ fprintf(stderr, "subd_shutdown called!\n"); abort(); }
 /* Generated stub for timer_expired */
 void timer_expired(tal_t *ctx UNNEEDED, struct timer *timer UNNEEDED)
 { fprintf(stderr, "timer_expired called!\n"); abort(); }
@@ -144,16 +163,6 @@ struct wallet *wallet_new(struct lightningd *ld UNNEEDED,
 { fprintf(stderr, "wallet_new called!\n"); abort(); }
 /* AUTOGENERATED MOCKS END */
 
-/* We only need these in developer mode */
-#if DEVELOPER
-/* Generated stub for opt_subd_debug */
-char *opt_subd_debug(const char *optarg UNNEEDED, struct lightningd *ld UNNEEDED)
-{ fprintf(stderr, "opt_subd_debug called!\n"); abort(); }
-/* Generated stub for opt_subd_dev_disconnect */
-char *opt_subd_dev_disconnect(const char *optarg UNNEEDED, struct lightningd *ld UNNEEDED)
-{ fprintf(stderr, "opt_subd_dev_disconnect called!\n"); abort(); }
-#endif
-
 struct log *crashlog;
 
 #undef main
@@ -166,26 +175,26 @@ int main(int argc UNUSED, char *argv[] UNUSED)
 	const char *answer;
 
 	setup_tmpctx();
-	answer = path_canon(tmpctx, "lightningd/test");
+	answer = path_canon(tmpctx, "lightningd/test/run-find_my_abspath");
 
 	/* Various different ways we could find ourselves. */
 	argv0 = path_join(tmpctx,
-			  path_cwd(tmpctx), "lightningd/test/run-find_my_path");
+			  path_cwd(tmpctx), "lightningd/test/run-find_my_abspath");
 	unsetenv("PATH");
 
 	/* Absolute path. */
-	assert(streq(find_my_path(tmpctx, argv0), answer));
+	assert(streq(find_my_abspath(tmpctx, argv0), answer));
 
 	/* Relative to cwd. */
-	argv0 = "lightningd/test/run-find_my_path";
-	assert(streq(find_my_path(tmpctx, argv0), answer));
+	argv0 = "lightningd/test/run-find_my_abspath";
+	assert(streq(find_my_abspath(tmpctx, argv0), answer));
 
 	/* Using $PATH */
 	setenv("PATH", path_join(tmpctx,
 				 path_cwd(tmpctx), "lightningd/test"), 1);
-	argv0 = "run-find_my_path";
+	argv0 = "run-find_my_abspath";
 
-	assert(streq(find_my_path(tmpctx, argv0), answer));
+	assert(streq(find_my_abspath(tmpctx, argv0), answer));
 
 	/* Even with dummy things in path. */
 	char **pathelems = tal_arr(tmpctx, char *, 4);
@@ -195,7 +204,7 @@ int main(int argc UNUSED, char *argv[] UNUSED)
 	pathelems[3] = NULL;
 
 	setenv("PATH", tal_strjoin(tmpctx, pathelems, ":", STR_NO_TRAIL), 1);
-	assert(streq(find_my_path(tmpctx, argv0), answer));
+	assert(streq(find_my_abspath(tmpctx, argv0), answer));
 
 	assert(!taken_any());
 	take_cleanup();