From a2c6ec6c9b190e6c24c0678c2138197235d03b52 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 18 Feb 2018 23:26:46 +1030 Subject: [PATCH] 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 --- lightningd/lightningd.c | 14 ++++---------- lightningd/lightningd.h | 3 ++- lightningd/log.c | 8 ++++---- lightningd/log.h | 6 +++--- lightningd/peer_control.c | 3 +-- lightningd/test/run-find_my_path.c | 3 +-- wallet/test/run-wallet.c | 3 +-- 7 files changed, 16 insertions(+), 24 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 10b722579..fa1f2365c 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -39,8 +39,7 @@ char *bitcoin_datadir; struct backtrace_state *backtrace_state; -static struct lightningd *new_lightningd(const tal_t *ctx, - struct log_book *log_book) +static struct lightningd *new_lightningd(const tal_t *ctx) { 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 = new_log(log_book, log_book, "lightningd(%u):", (int)getpid()); + ld->log_book = new_log_book(20*1024*1024, LOG_INFORM); + 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 */ - log_book = new_log_book(NULL, 20*1024*1024, LOG_INFORM); - ld = new_lightningd(NULL, log_book); - + ld = new_lightningd(NULL); 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(); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index d9171b46d..064dc24f7 100644 --- a/lightningd/lightningd.h +++ b/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; diff --git a/lightningd/log.c b/lightningd/log.c index a5205e2cc..3a8d15976 100644 --- a/lightningd/log.c +++ b/lightningd/log.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -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, - size_t max_mem, +struct log_book *new_log_book(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! */ diff --git a/lightningd/log.h b/lightningd/log.h index 3082fc5eb..7bb0323f9 100644 --- a/lightningd/log.h +++ b/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. */ -struct log_book *new_log_book(const tal_t *ctx, - size_t max_mem, +/* We can have a single log book, with multiple logs in it: it's freed by + * the last struct log itself. */ +struct log_book *new_log_book(size_t max_mem, enum log_level printlevel); /* With different entry points */ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 8ec12aeed..a1dd345b8 100644 --- a/lightningd/peer_control.c +++ b/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, - get_log_level(ld->log_book)); + peer->log_book = new_log_book(128*1024, 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); diff --git a/lightningd/test/run-find_my_path.c b/lightningd/test/run-find_my_path.c index 288836f3e..9c545b690 100644 --- a/lightningd/test/run-find_my_path.c +++ b/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, - size_t max_mem UNNEEDED, +struct log_book *new_log_book(size_t max_mem UNNEEDED, enum log_level printlevel UNNEEDED) { fprintf(stderr, "new_log_book called!\n"); abort(); } /* Generated stub for new_topology */ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 538b9c97f..b65bd39bf 100644 --- a/wallet/test/run-wallet.c +++ b/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, - size_t max_mem UNNEEDED, +struct log_book *new_log_book(size_t max_mem UNNEEDED, enum log_level printlevel UNNEEDED) { return NULL;