common/daemon.c: remove #ifdef DEVELOPER in favor of runtime flag.

Also requires us to expose memleak when !DEVELOPER, however we only ever used the memleak tracking when the LIGHTNINGD_DEV_MEMLEAK environment variable was set, so keep that. Signed-off-by: Rusty Russell <[email protected]>
gudnuf · Sep 21, 2023 · a9f26b7 · a9f26b7
1 parent 67a391f
commit a9f26b7
Show file tree

Hide file tree

Showing 21 changed files with 108 additions and 129 deletions.
diff --git a/channeld/full_channel.c b/channeld/full_channel.c
@@ -18,13 +18,11 @@
   /* Needs to be at end, since it doesn't include its own hdrs */
   #include "full_channel_error_names_gen.h"
 
-#if DEVELOPER
 static void memleak_help_htlcmap(struct htable *memtable,
 				 struct htlc_map *htlcs)
 {
 	memleak_scan_htable(memtable, &htlcs->raw);
 }
-#endif /* DEVELOPER */
 
 /* This is a dangerous thing!  Because we apply HTLCs in many places
  * in bulk, we can temporarily go negative.  You must check balance_ok()

diff --git a/common/daemon.c b/common/daemon.c
@@ -96,34 +96,7 @@ static void crashlog_activate(void)
 	sigaction(SIGSEGV, &sa, NULL);
 	sigaction(SIGBUS, &sa, NULL);
 }
-#else
-void send_backtrace(const char *why)
-{
-}
-
-const char *backtrace_symname(const tal_t *ctx, const void *addr)
-{
-	return "unknown (backtrace unsupported)";
-}
-#endif
-
-int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout)
-{
-	const char *t;
-
-	t = taken_any();
-	if (t)
-		errx(1, "Outstanding taken pointers: %s", t);
-
-	if (wally_tal_ctx)
-		errx(1, "Outstanding tal_wally_start!");
-
-	clean_tmpctx();
-
-	return poll(fds, nfds, timeout);
-}
 
-#if DEVELOPER && BACKTRACE_SUPPORTED
 static void steal_notify(tal_t *child, enum tal_notify_type n, tal_t *newparent)
 {
 	tal_t *p = newparent;
@@ -154,8 +127,33 @@ static void remove_steal_notifiers(void)
 	/* We remove this from root, assuming everything else freed. */
 	tal_del_notifier(NULL, add_steal_notifier);
 }
+#else
+void send_backtrace(const char *why)
+{
+}
+
+const char *backtrace_symname(const tal_t *ctx, const void *addr)
+{
+	return "unknown (backtrace unsupported)";
+}
 #endif
 
+int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout)
+{
+	const char *t;
+
+	t = taken_any();
+	if (t)
+		errx(1, "Outstanding taken pointers: %s", t);
+
+	if (wally_tal_ctx)
+		errx(1, "Outstanding tal_wally_start!");
+
+	clean_tmpctx();
+
+	return poll(fds, nfds, timeout);
+}
+
 void daemon_setup(const char *argv0,
 		  void (*backtrace_print)(const char *fmt, ...),
 		  void (*backtrace_exit)(void))
@@ -164,22 +162,14 @@ void daemon_setup(const char *argv0,
 #if BACKTRACE_SUPPORTED
 	bt_print = backtrace_print;
 	bt_exit = backtrace_exit;
-#if DEVELOPER
+
 	/* Suppresses backtrace (breaks valgrind) */
 	if (!getenv("LIGHTNINGD_DEV_NO_BACKTRACE"))
 		backtrace_state = backtrace_create_state(argv0, 0, NULL, NULL);
-	add_steal_notifiers(NULL);
-#else
-	backtrace_state = backtrace_create_state(argv0, 0, NULL, NULL);
-#endif
 	crashlog_activate();
 #endif
 
-#if DEVELOPER
-	/* This has significant overhead, so we only enable it if told */
-	if (getenv("LIGHTNINGD_DEV_MEMLEAK"))
-		memleak_init();
-#endif
+	memleak_init();
 
 	/* We handle write returning errors! */
 	signal(SIGPIPE, SIG_IGN);
@@ -191,18 +181,27 @@ void daemon_shutdown(void)
 {
 	common_shutdown();
 
-#if DEVELOPER && BACKTRACE_SUPPORTED
+#if BACKTRACE_SUPPORTED
+	/* Harmless if it wasn't applied */
 	remove_steal_notifiers();
 #endif
 }
 
-void daemon_maybe_debug(char *argv[])
+bool daemon_developer_mode(char *argv[])
 {
-#if DEVELOPER
+	bool developer = false, debug = false;
+
 	for (int i = 1; argv[i]; i++) {
-		if (!streq(argv[i], "--debugger"))
-			continue;
+		if (streq(argv[i], "--debugger"))
+			debug = true;
+		else if (streq(argv[i], "--developer"))
+			developer = true;
+	}
+
+	if (!developer)
+		return false;
 
+	if (debug) {
 		/* Don't let this mess up stdout, so redir to /dev/null */
 		char *cmd = tal_fmt(NULL, "${DEBUG_TERM:-gnome-terminal --} gdb -ex 'attach %u' %s >/dev/null &", getpid(), argv[0]);
 		fprintf(stderr, "Running %s\n", cmd);
@@ -214,5 +213,9 @@ void daemon_maybe_debug(char *argv[])
 		/* Continue in the debugger. */
 		kill(getpid(), SIGSTOP);
 	}
-#endif /* DEVELOPER */
+
+	/* This checks for any tal_steal loops! */
+	add_steal_notifiers(NULL);
+
+	return true;
 }
diff --git a/common/daemon.h b/common/daemon.h
@@ -21,7 +21,8 @@ const char *backtrace_symname(const tal_t *ctx, const void *addr);
 /* Shutdown for a valgrind-clean exit (frees everything) */
 void daemon_shutdown(void);
 
-/* Kick in a debugger if they set --debugger */
-void daemon_maybe_debug(char *argv[]);
+/* If --developer is set, set up extra developer checks, kick in a
+ * debugger if they set --debugger, and return true.   */
+bool daemon_developer_mode(char *argv[]);
 
 #endif /* LIGHTNING_COMMON_DAEMON_H */
diff --git a/common/memleak.c b/common/memleak.c
@@ -7,7 +7,7 @@
  * but tal hierarchies tends to get freed at exit, so we need something
  * more sophisticated).
  *
- * Memleak detection is only active if DEVELOPER is set.  It does several
+ * Memleak detection is only active if $LIGHTNINGD_DEV_MEMLEAK is set.  It does several
  * things:
  * 1. attaches a backtrace list to every allocation, so we can tell
  *    where it came from.
@@ -32,7 +32,6 @@
 
 struct backtrace_state *backtrace_state;
 
-#if DEVELOPER
 static bool memleak_track;
 
 struct memleak_helper {
@@ -324,9 +323,11 @@ struct htable *memleak_start(const tal_t *ctx)
 
 void memleak_init(void)
 {
-	memleak_track = true;
-	if (backtrace_state)
-		add_backtrace_notifiers(NULL);
+	if (getenv("LIGHTNINGD_DEV_MEMLEAK")) {
+		memleak_track = true;
+		if (backtrace_state)
+			add_backtrace_notifiers(NULL);
+	}
 }
 
 static int dump_syminfo(void *data, uintptr_t pc UNUSED,
@@ -381,9 +382,3 @@ bool dump_memleak(struct htable *memtable,
 
 	return found_leak;
 }
-#else /* !DEVELOPER */
-void *notleak_(void *ptr, bool plus_children UNNEEDED)
-{
-	return ptr;
-}
-#endif /* !DEVELOPER */
diff --git a/common/memleak.h b/common/memleak.h
@@ -38,7 +38,6 @@ void memleak_init(void);
 
 void *notleak_(void *ptr, bool plus_children);
 
-#if DEVELOPER
 /**
  * memleak_add_helper: help memleak look inside this tal object
  * @p: the tal object
@@ -55,10 +54,6 @@ void *notleak_(void *ptr, bool plus_children);
 			    typesafe_cb_preargs(void, const tal_t *,	\
 						(cb), (p),		\
 						struct htable *))
-#else
-/* Don't refer to cb at all if !DEVELOPER */
-#define memleak_add_helper(p, cb)
-#endif
 
 /* For update-mock: memleak_add_helper_ mock empty */
 void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable,
@@ -147,10 +142,7 @@ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace);
 
 extern struct backtrace_state *backtrace_state;
 
-#if DEVELOPER
-/* Only defined if DEVELOPER */
 bool dump_memleak(struct htable *memtable,
 		  void PRINTF_FMT(1,2) (*print)(const char *fmt, ...));
-#endif
 
 #endif /* LIGHTNING_COMMON_MEMLEAK_H */
diff --git a/common/status.c b/common/status.c
@@ -234,7 +234,6 @@ void master_badmsg(u32 type_expected, const u8 *msg)
 		     type_expected, tal_hex(tmpctx, msg));
 }
 
-#if DEVELOPER
 /* Print BROKEN status: callback for dump_memleak. */
 void memleak_status_broken(const char *fmt, ...)
 {
@@ -243,4 +242,3 @@ void memleak_status_broken(const char *fmt, ...)
 	status_vfmt(LOG_BROKEN, NULL, fmt, ap);
 	va_end(ap);
 }
-#endif
diff --git a/common/status.h b/common/status.h
@@ -70,9 +70,7 @@ void status_send_fatal(const u8 *msg TAKES) NORETURN;
 /* Only for sync status! */
 void status_send_fd(int fd);
 
-#if DEVELOPER
 /* Print BROKEN status: callback for dump_memleak. */
 void memleak_status_broken(const char *fmt, ...);
-#endif
 
 #endif /* LIGHTNING_COMMON_STATUS_H */
diff --git a/common/subdaemon.c b/common/subdaemon.c
@@ -18,8 +18,10 @@ static void status_backtrace_exit(void)
 	status_failed(STATUS_FAIL_INTERNAL_ERROR, "FATAL SIGNAL");
 }
 
-void subdaemon_setup(int argc, char *argv[])
+bool subdaemon_setup(int argc, char *argv[])
 {
+	bool developer;
+
 	if (argc == 2 && streq(argv[1], "--version")) {
 		printf("%s\n", version());
 		exit(0);
@@ -30,7 +32,7 @@ void subdaemon_setup(int argc, char *argv[])
 			logging_io = true;
 	}
 
-	daemon_maybe_debug(argv);
-
+	developer = daemon_developer_mode(argv);
 	daemon_setup(argv[0], status_backtrace_print, status_backtrace_exit);
+	return developer;
 }
diff --git a/common/subdaemon.h b/common/subdaemon.h
@@ -5,7 +5,7 @@
 
 struct htable;
 
-/* daemon_setup, but for subdaemons */
-void subdaemon_setup(int argc, char *argv[]);
+/* daemon_setup, but for subdaemons: returns true if --developer */
+bool subdaemon_setup(int argc, char *argv[]);
 
 #endif /* LIGHTNING_COMMON_SUBDAEMON_H */
diff --git a/connectd/connectd.c b/connectd/connectd.c
@@ -1862,7 +1862,6 @@ static void peer_final_msg(struct io_conn *conn,
 		multiplex_final_msg(peer, take(finalmsg));
 }
 
-#if DEVELOPER
 static void dev_connect_memleak(struct daemon *daemon, const u8 *msg)
 {
 	struct htable *memtable;
@@ -1881,6 +1880,7 @@ static void dev_connect_memleak(struct daemon *daemon, const u8 *msg)
 							      found_leak)));
 }
 
+#if DEVELOPER
 static void dev_suppress_gossip(struct daemon *daemon, const u8 *msg)
 {
 	daemon->dev_suppress_gossip = true;
@@ -2082,10 +2082,11 @@ static struct io_plan *recv_req(struct io_conn *conn,
 		goto out;
 
 	case WIRE_CONNECTD_DEV_MEMLEAK:
-#if DEVELOPER
-		dev_connect_memleak(daemon, msg);
-		goto out;
-#endif
+		if (daemon->developer) {
+			dev_connect_memleak(daemon, msg);
+			goto out;
+		}
+		/* Fall thru */
 	case WIRE_CONNECTD_DEV_SUPPRESS_GOSSIP:
 #if DEVELOPER
 		dev_suppress_gossip(daemon, msg);
@@ -2152,14 +2153,12 @@ static struct io_plan *recv_gossip(struct io_conn *conn,
 	return daemon_conn_read_next(conn, daemon->gossipd);
 }
 
-/*~ This is a hook used by the memleak code (if DEVELOPER=1): it can't see
- * pointers inside hash tables, so we give it a hint here. */
-#if DEVELOPER
+/*~ This is a hook used by the memleak code: it can't see pointers
+ * inside hash tables, so we give it a hint here. */
 static void memleak_daemon_cb(struct htable *memtable, struct daemon *daemon)
 {
 	memleak_scan_htable(memtable, &daemon->peers->raw);
 }
-#endif /* DEVELOPER */
 
 static void gossipd_failed(struct daemon_conn *gossipd)
 {
@@ -2168,15 +2167,17 @@ static void gossipd_failed(struct daemon_conn *gossipd)
 
 int main(int argc, char *argv[])
 {
-	setup_locale();
-
 	struct daemon *daemon;
+	bool developer;
+
+	setup_locale();
 
 	/* Common subdaemon setup code. */
-	subdaemon_setup(argc, argv);
+	developer = subdaemon_setup(argc, argv);
 
 	/* Allocate and set up our simple top-level structure. */
 	daemon = tal(NULL, struct daemon);
+	daemon->developer = developer;
 	daemon->connection_counter = 1;
 	daemon->peers = tal(daemon, struct peer_htable);
 	daemon->listeners = tal_arr(daemon, struct io_listener *, 0);

diff --git a/connectd/connectd.h b/connectd/connectd.h
@@ -126,6 +126,9 @@ struct daemon {
 	/* Who am I? */
 	struct node_id id;
 
+	/* --developer? */
+	bool developer;
+
 	/* pubkey equivalent. */
 	struct pubkey mykey;
 

diff --git a/gossipd/routing.c b/gossipd/routing.c
@@ -227,7 +227,6 @@ static bool timestamp_reasonable(struct routing_state *rstate, u32 timestamp)
 	return true;
 }
 
-#if DEVELOPER
 static void memleak_help_routing_tables(struct htable *memtable,
 					struct routing_state *rstate)
 {
@@ -246,7 +245,6 @@ static void memleak_help_routing_tables(struct htable *memtable,
 			memleak_scan_htable(memtable, &n->chan_map->raw);
 	}
 }
-#endif /* DEVELOPER */
 
 /* Once an hour, or at 10000 entries, we expire old ones */
 static void txout_failure_age(struct routing_state *rstate)

diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
@@ -1239,13 +1239,11 @@ static void destroy_jsonrpc(struct jsonrpc *jsonrpc)
 	strmap_clear(&jsonrpc->usagemap);
 }
 
-#if DEVELOPER
 static void memleak_help_jsonrpc(struct htable *memtable,
 				 struct jsonrpc *jsonrpc)
 {
 	memleak_scan_strmap(memtable, &jsonrpc->usagemap);
 }
-#endif /* DEVELOPER */
 
 void jsonrpc_setup(struct lightningd *ld)
 {