lightningd: use tal_link for log_book.

BackgroundL Each log has a log_book: many logs can share the same one,
as each one can have a separate prefix.

Testing tickled a bug at the end of this series, where subd was
logging to the peer's log_book on shutdown, but the peer was already
freed.  We've already had issues with logging while lightningd is
shutting down.

There are times when reference counting really is the right answer,
this seems to be one of them: the 'struct log' share the 'struct
log_book' and the last 'struct log' cleans it up.

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

lightningd/lightningd.c

@@ -39,8 +39,7 @@ char *bitcoin_datadir;
 
 struct backtrace_state *backtrace_state;
 
-static struct lightningd *new_lightningd(const tal_t *ctx,
+static struct lightningd *new_lightningd(const tal_t *ctx)
-					 struct log_book *log_book)
 {
 	struct lightningd *ld = tal(ctx, struct lightningd);
 
@@ -58,8 +57,8 @@ static struct lightningd *new_lightningd(const tal_t *ctx,
 	list_head_init(&ld->peers);
 	htlc_in_map_init(&ld->htlcs_in);
 	htlc_out_map_init(&ld->htlcs_out);
-	ld->log_book = log_book;
+	ld->log_book = new_log_book(20*1024*1024, LOG_INFORM);
-	ld->log = new_log(log_book, log_book, "lightningd(%u):", (int)getpid());
+	ld->log = new_log(ld, ld->log_book, "lightningd(%u):", (int)getpid());
 	ld->logfile = NULL;
 	ld->alias = NULL;
 	ld->rgb = NULL;
@@ -253,7 +252,6 @@ static void daemonize_but_keep_dir(void)
 
 int main(int argc, char *argv[])
 {
-	struct log_book *log_book;
 	struct lightningd *ld;
 	bool newdir;
 	u32 first_blocknum;
@@ -266,10 +264,7 @@ int main(int argc, char *argv[])
 #endif
 	backtrace_state = backtrace_create_state(argv[0], 0, NULL, NULL);
 
-	/* Things log on shutdown, so we need this to outlive lightningd */
+	ld = new_lightningd(NULL);
-	log_book = new_log_book(NULL, 20*1024*1024, LOG_INFORM);
-	ld = new_lightningd(NULL, log_book);
-
 	secp256k1_ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY
 						 | SECP256K1_CONTEXT_SIGN);
 
@@ -392,7 +387,6 @@ int main(int argc, char *argv[])
 
 	tal_free(ld);
 	opt_free_table();
-	tal_free(log_book);
 
 #if DEVELOPER
 	memleak_cleanup();

lightningd/lightningd.h

@@ -87,8 +87,9 @@ struct lightningd {
 	/* Configuration settings. */
 	struct config config;
 
-	/* Log for general stuff. */
+	/* This log_book is owned by all the struct logs */
 	struct log_book *log_book;
+	/* Log for general stuff. */
 	struct log *log;
 	const char *logfile;
 

lightningd/log.c

@@ -5,6 +5,7 @@
 #include <ccan/opt/opt.h>
 #include <ccan/read_write_all/read_write_all.h>
 #include <ccan/str/hex/hex.h>
+#include <ccan/tal/link/link.h>
 #include <ccan/tal/str/str.h>
 #include <ccan/time/time.h>
 #include <common/memleak.h>
@@ -122,11 +123,10 @@ static size_t prune_log(struct log_book *log)
 	return deleted;
 }
 
-struct log_book *new_log_book(const tal_t *ctx,
+struct log_book *new_log_book(size_t max_mem,
-			      size_t max_mem,
 			      enum log_level printlevel)
 {
-	struct log_book *lr = tal(ctx, struct log_book);
+	struct log_book *lr = tal_linkable(tal(NULL, struct log_book));
 
 	/* Give a reasonable size for memory limit! */
 	assert(max_mem > sizeof(struct log) * 2);
@@ -151,7 +151,7 @@ new_log(const tal_t *ctx, struct log_book *record, const char *fmt, ...)
 	struct log *log = tal(ctx, struct log);
 	va_list ap;
 
-	log->lr = record;
+	log->lr = tal_link(log, record);
 	va_start(ap, fmt);
 	/* log->lr owns this, since its entries keep a pointer to it. */
 	/* FIXME: Refcount this! */

lightningd/log.h

@@ -13,9 +13,9 @@ struct json_result;
 struct lightningd;
 struct timerel;
 
-/* We can have a single log book, with multiple logs in it. */
+/* We can have a single log book, with multiple logs in it: it's freed by
-struct log_book *new_log_book(const tal_t *ctx,
+ * the last struct log itself. */
-			      size_t max_mem,
+struct log_book *new_log_book(size_t max_mem,
 			      enum log_level printlevel);
 
 /* With different entry points */

lightningd/peer_control.c

@@ -112,8 +112,7 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid,
 	peer->direction = get_channel_direction(&peer->ld->id, &peer->id);
 
 	/* Max 128k per peer. */
-	peer->log_book = new_log_book(peer, 128*1024,
+	peer->log_book = new_log_book(128*1024, get_log_level(ld->log_book));
-				      get_log_level(ld->log_book));
 	set_log_outfn(peer->log_book, copy_to_parent_log, peer);
 	list_add_tail(&ld->peers, &peer->list);
 	tal_add_destructor(peer, destroy_peer);

lightningd/test/run-find_my_path.c

@@ -51,8 +51,7 @@ void log_(struct log *log UNNEEDED, enum log_level level UNNEEDED, const char *f
 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(); }
 /* Generated stub for new_log_book */
-struct log_book *new_log_book(const tal_t *ctx UNNEEDED,
+struct log_book *new_log_book(size_t max_mem UNNEEDED,
-			      size_t max_mem UNNEEDED,
 			      enum log_level printlevel UNNEEDED)
 { fprintf(stderr, "new_log_book called!\n"); abort(); }
 /* Generated stub for new_topology */

wallet/test/run-wallet.c

@@ -556,8 +556,7 @@ struct log *new_log(const tal_t *ctx UNNEEDED, struct log_book *record UNNEEDED,
 	return NULL;
 }
 
-struct log_book *new_log_book(const tal_t *ctx UNNEEDED,
+struct log_book *new_log_book(size_t max_mem UNNEEDED,
-			      size_t max_mem UNNEEDED,
 			      enum log_level printlevel UNNEEDED)
 {
 	return NULL;