From d2f691b288eb704f91271f2b719491dc3855c688 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 18 Feb 2018 23:29:46 +1030 Subject: [PATCH] subd: make functions more generic, don't assume 'struct channel'. This means the caller needs to supply an explicit log to base the subd log on, and also a callback for error handling. The callback is kind of ugly, but it gets reworked towards the end of this series. Signed-off-by: Rusty Russell --- lightningd/log.c | 5 +++ lightningd/log.h | 2 ++ lightningd/peer_control.c | 46 ++++++++++++++++++++++++++- lightningd/subd.c | 67 +++++++++++++++++++++++---------------- lightningd/subd.h | 44 ++++++++++++++++++------- wallet/test/run-wallet.c | 26 +++++++++------ 6 files changed, 141 insertions(+), 49 deletions(-) diff --git a/lightningd/log.c b/lightningd/log.c index 3a8d15976..6b3dc1c89 100644 --- a/lightningd/log.c +++ b/lightningd/log.c @@ -161,6 +161,11 @@ new_log(const tal_t *ctx, struct log_book *record, const char *fmt, ...) return log; } +struct log_book *get_log_book(const struct log *log) +{ + return log->lr; +} + enum log_level get_log_level(struct log_book *lr) { return lr->print_level; diff --git a/lightningd/log.h b/lightningd/log.h index 7bb0323f9..4be6b3be3 100644 --- a/lightningd/log.h +++ b/lightningd/log.h @@ -39,6 +39,8 @@ enum log_level get_log_level(struct log_book *lr); void set_log_level(struct log_book *lr, enum log_level level); void set_log_prefix(struct log *log, const char *prefix); const char *log_prefix(const struct log *log); +struct log_book *get_log_book(const struct log *log); + #define set_log_outfn(lr, print, arg) \ set_log_outfn_((lr), \ typesafe_cb_preargs(void, void *, (print), (arg),\ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index ad5230ec6..a2d33e0bf 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -277,6 +277,29 @@ static void channel_config(struct lightningd *ld, ours->channel_reserve_satoshis = 0; }; +static void channel_errmsg(struct channel *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg) +{ + if (sender == LOCAL) { + /* If this is NULL, it means subd died. */ + if (!errmsg) { + channel_fail_transient(channel, "%s", desc); + return; + } + + /* Otherwise overwrite any error we have */ + if (!channel->error) + channel->error = tal_dup_arr(channel, u8, + errmsg, tal_len(errmsg), 0); + } + + channel_fail_permanent(channel, "%s ERROR %s", + sender == LOCAL ? "sent" : "received", desc); +} + /* Gossipd tells us a peer has connected */ void peer_connected(struct lightningd *ld, const u8 *msg, int peer_fd, int gossip_fd) @@ -1229,6 +1252,17 @@ static bool tell_if_missing(const struct channel *channel, return true; } +/* Only error onchaind can get is if it dies. */ +static void onchain_error(struct channel *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg) +{ + /* FIXME: re-launch? */ + log_broken(channel->log, "%s", desc); +} + /* With a reorg, this can get called multiple times; each time we'll kill * onchaind (like any other owner), and restart */ static enum watch_result funding_spent(struct channel *channel, @@ -1253,8 +1287,10 @@ static enum watch_result funding_spent(struct channel *channel, channel_set_owner(channel, new_channel_subd(ld, "lightning_onchaind", channel, + channel->log, onchain_wire_type_name, onchain_msg, + onchain_error, NULL)); if (!channel->owner) { @@ -1720,8 +1756,10 @@ static void peer_start_closingd(struct channel *channel, } channel_set_owner(channel, new_channel_subd(ld, - "lightning_closingd", channel, + "lightning_closingd", + channel, channel->log, closing_wire_type_name, closing_msg, + channel_errmsg, take(&peer_fd), take(&gossip_fd), NULL)); if (!channel->owner) { @@ -1946,8 +1984,10 @@ static bool peer_start_channeld(struct channel *channel, channel_set_owner(channel, new_channel_subd(ld, "lightning_channeld", channel, + channel->log, channel_wire_type_name, channel_msg, + channel_errmsg, take(&peer_fd), take(&gossip_fd), take(&hsmfd), NULL)); @@ -2325,8 +2365,10 @@ static void peer_accept_channel(struct lightningd *ld, channel_set_state(channel, UNINITIALIZED, OPENINGD); channel_set_owner(channel, new_channel_subd(ld, "lightning_openingd", channel, + channel->log, opening_wire_type_name, opening_negotiation_failed, + channel_errmsg, take(&peer_fd), take(&gossip_fd), NULL)); if (!channel->owner) { @@ -2394,8 +2436,10 @@ static void peer_offer_channel(struct lightningd *ld, channel_set_owner(fc->channel, new_channel_subd(ld, "lightning_openingd", fc->channel, + fc->channel->log, opening_wire_type_name, opening_negotiation_failed, + channel_errmsg, take(&peer_fd), take(&gossip_fd), NULL)); if (!fc->channel->owner) { diff --git a/lightningd/subd.c b/lightningd/subd.c index b0af1def9..21f81403b 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -389,7 +388,7 @@ static bool log_status_fail(struct subd *sd, const u8 *msg) static bool handle_received_errmsg(struct subd *sd, const u8 *msg) { - struct channel *channel = sd->channel; + void *channel = sd->channel; struct channel_id channel_id; char *desc; @@ -401,14 +400,14 @@ static bool handle_received_errmsg(struct subd *sd, const u8 *msg) /* Don't free sd; we're may be about to free channel. */ sd->channel = NULL; - channel_fail_permanent(channel, - "%s: received ERROR %s", sd->name, desc); + if (sd->errcb) + sd->errcb(channel, REMOTE, &channel_id, desc, NULL); return true; } static bool handle_sent_errmsg(struct subd *sd, const u8 *msg) { - struct channel *channel = sd->channel; + void *channel = sd->channel; struct channel_id channel_id; char *desc; u8 *errmsg; @@ -418,13 +417,11 @@ static bool handle_sent_errmsg(struct subd *sd, const u8 *msg) return false; /* FIXME: if not all channels failed, hand back to gossipd! */ - if (!channel->error) - channel->error = tal_steal(channel, errmsg); /* Don't free sd; we're may be about to free channel. */ sd->channel = NULL; - channel_fail_permanent(channel, - "%s: sent ERROR %s", sd->name, desc); + if (sd->errcb) + sd->errcb(channel, LOCAL, &channel_id, desc, errmsg); return true; } @@ -573,7 +570,7 @@ static void destroy_subd(struct subd *sd) /* Peer still attached? */ if (sd->channel) { /* Don't loop back when we fail it. */ - struct channel *channel = sd->channel; + void *channel = sd->channel; struct db *db = sd->ld->wallet->db; bool outer_transaction; @@ -583,9 +580,11 @@ static void destroy_subd(struct subd *sd) outer_transaction = db->in_transaction; if (!outer_transaction) db_begin_transaction(db); - channel_fail_transient(channel, - "Owning subdaemon %s died (%i)", - sd->name, status); + if (sd->errcb) + sd->errcb(channel, LOCAL, NULL, + tal_fmt(sd, "Owning subdaemon %s died (%i)", + sd->name, status), + NULL); if (!outer_transaction) db_commit_transaction(db); } @@ -625,10 +624,16 @@ static struct io_plan *msg_setup(struct io_conn *conn, struct subd *sd) static struct subd *new_subd(struct lightningd *ld, const char *name, - struct channel *channel, + void *channel, + struct log *base_log, const char *(*msgname)(int msgtype), unsigned int (*msgcb)(struct subd *, const u8 *, const int *fds), + void (*errcb)(void *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg), va_list *ap) { struct subd *sd = tal(ld, struct subd); @@ -649,9 +654,9 @@ static struct subd *new_subd(struct lightningd *ld, return tal_free(sd); } sd->ld = ld; - if (channel) { - sd->log = new_log(sd, channel->peer->log_book, "%s-%s", name, - log_prefix(channel->log)); + if (base_log) { + sd->log = new_log(sd, get_log_book(base_log), "%s-%s", name, + log_prefix(base_log)); } else { sd->log = new_log(sd, ld->log_book, "%s(%u):", name, sd->pid); } @@ -660,6 +665,7 @@ static struct subd *new_subd(struct lightningd *ld, sd->must_not_exit = false; sd->msgname = msgname; sd->msgcb = msgcb; + sd->errcb = errcb; sd->fds_in = NULL; msg_queue_init(&sd->outq, sd); tal_add_destructor(sd, destroy_subd); @@ -686,25 +692,32 @@ struct subd *new_global_subd(struct lightningd *ld, struct subd *sd; va_start(ap, msgcb); - sd = new_subd(ld, name, NULL, msgname, msgcb, &ap); + sd = new_subd(ld, name, NULL, NULL, msgname, msgcb, NULL, &ap); va_end(ap); sd->must_not_exit = true; return sd; } -struct subd *new_channel_subd(struct lightningd *ld, - const char *name, - struct channel *channel, - const char *(*msgname)(int msgtype), - unsigned int (*msgcb)(struct subd *, const u8 *, - const int *fds), ...) +struct subd *new_channel_subd_(struct lightningd *ld, + const char *name, + void *channel, + struct log *base_log, + const char *(*msgname)(int msgtype), + unsigned int (*msgcb)(struct subd *, const u8 *, + const int *fds), + void (*errcb)(void *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg), + ...) { va_list ap; struct subd *sd; - va_start(ap, msgcb); - sd = new_subd(ld, name, channel, msgname, msgcb, &ap); + va_start(ap, errcb); + sd = new_subd(ld, name, channel, base_log, msgname, msgcb, errcb, &ap); va_end(ap); return sd; } @@ -764,7 +777,7 @@ void subd_shutdown(struct subd *sd, unsigned int seconds) tal_free(sd); } -void subd_release_channel(struct subd *owner, struct channel *channel) +void subd_release_channel(struct subd *owner, void *channel) { /* If owner is a per-peer-daemon, and not already freeing itself... */ if (owner->channel) { diff --git a/lightningd/subd.h b/lightningd/subd.h index 6cf338f74..1177e18f0 100644 --- a/lightningd/subd.h +++ b/lightningd/subd.h @@ -5,6 +5,7 @@ #include #include #include +#include #include struct io_conn; @@ -26,7 +27,7 @@ struct subd { struct io_conn *conn; /* If we are associated with a single channel, this points to it. */ - struct channel *channel; + void *channel; /* For logging */ struct log *log; @@ -35,6 +36,13 @@ struct subd { unsigned (*msgcb)(struct subd *, const u8 *, const int *); const char *(*msgname)(int msgtype); + /* Callback when an errormsg sent/received, or subd died. */ + void (*errcb)(void *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg); + /* Buffer for input. */ u8 *msg_in; @@ -77,6 +85,7 @@ struct subd *new_global_subd(struct lightningd *ld, * @ld: global state * @name: basename of daemon * @channel: channel to associate. + * @base_log: log to use (actually makes a copy so it has name in prefix) * @msgname: function to get name from messages * @msgcb: function to call (inside db transaction) when non-fatal message received (or NULL) * @...: NULL-terminated list of pointers to fds to hand as fd 3, 4... @@ -86,14 +95,27 @@ struct subd *new_global_subd(struct lightningd *ld, * that many @fds are received before calling again. If it returns -1, the * subdaemon is shutdown. */ -struct subd *new_channel_subd(struct lightningd *ld, - const char *name, - struct channel *channel, - const char *(*msgname)(int msgtype), - unsigned int (*msgcb)(struct subd *, const u8 *, - const int *fds), - ...); - +struct subd *new_channel_subd_(struct lightningd *ld, + const char *name, + void *channel, + struct log *base_log, + const char *(*msgname)(int msgtype), + unsigned int (*msgcb)(struct subd *, const u8 *, + const int *fds), + void (*errcb)(void *channel, + enum side sender, + const struct channel_id *channel_id, + const char *desc, + const u8 *errmsg), + ...); + +#define new_channel_subd(ld, name, channel, log, msgname, msgcb, errcb, ...) \ + new_channel_subd_((ld), (name), (channel), (log), (msgname), (msgcb), \ + typesafe_cb_postargs(void, void *, (errcb), \ + (channel), enum side, \ + const struct channel_id *, \ + const char *, const u8 *), \ + __VA_ARGS__) /** * subd_raw - raw interface to get a subdaemon on an fd (for HSM) * @ld: global state @@ -144,14 +166,14 @@ void subd_req_(const tal_t *ctx, void *replycb_data); /** - * subd_release_channel - shut down a subdaemon which no longer owns the channel. + * subd_release_channel - shut down a subdaemon which no longer owns the channe;. * @owner: subd which owned channel. * @channel: channel to release. * * If the subdaemon is not already shutting down, and it is a per-channel * subdaemon, this shuts it down. */ -void subd_release_channel(struct subd *owner, struct channel *channel); +void subd_release_channel(struct subd *owner, void *channel); /** * subd_shutdown - try to politely shut down a subdaemon. diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index b65bd39bf..1f3870d64 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -325,15 +325,21 @@ void log_io(struct log *log UNNEEDED, enum log_level dir UNNEEDED, const char *c /* Generated stub for logv_add */ void logv_add(struct log *log UNNEEDED, const char *fmt UNNEEDED, va_list ap UNNEEDED) { fprintf(stderr, "logv_add called!\n"); abort(); } -/* Generated stub for new_channel_subd */ -struct subd *new_channel_subd(struct lightningd *ld UNNEEDED, - const char *name UNNEEDED, - struct channel *channel UNNEEDED, - const char *(*msgname)(int msgtype) UNNEEDED, - unsigned int (*msgcb)(struct subd * UNNEEDED, const u8 * UNNEEDED, - const int *fds) UNNEEDED, - ...) -{ fprintf(stderr, "new_channel_subd called!\n"); abort(); } +/* Generated stub for new_channel_subd_ */ +struct subd *new_channel_subd_(struct lightningd *ld UNNEEDED, + const char *name UNNEEDED, + void *channel UNNEEDED, + struct log *base_log UNNEEDED, + const char *(*msgname)(int msgtype) UNNEEDED, + unsigned int (*msgcb)(struct subd * UNNEEDED, const u8 * UNNEEDED, + const int *fds) UNNEEDED, + void (*errcb)(void *channel UNNEEDED, + enum side sender UNNEEDED, + const struct channel_id *channel_id UNNEEDED, + const char *desc UNNEEDED, + const u8 *errmsg) UNNEEDED, + ...) +{ fprintf(stderr, "new_channel_subd_ called!\n"); abort(); } /* Generated stub for new_json_result */ struct json_result *new_json_result(const tal_t *ctx UNNEEDED) { fprintf(stderr, "new_json_result called!\n"); abort(); } @@ -379,7 +385,7 @@ char *sanitize_error(const tal_t *ctx UNNEEDED, const u8 *errmsg UNNEEDED, struct channel_id *channel_id UNNEEDED) { fprintf(stderr, "sanitize_error called!\n"); abort(); } /* Generated stub for subd_release_channel */ -void subd_release_channel(struct subd *owner UNNEEDED, struct channel *channel UNNEEDED) +void subd_release_channel(struct subd *owner UNNEEDED, void *channel UNNEEDED) { fprintf(stderr, "subd_release_channel called!\n"); abort(); } /* Generated stub for subd_req_ */ void subd_req_(const tal_t *ctx UNNEEDED,