From f48adb097e4d267bcc8242f3e7a4915cf7cb238b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 22 Jan 2016 06:41:46 +1030 Subject: [PATCH] state: use peer_unexpected_pkt() for an unexpected packet. Instead of effect->in_error. Signed-off-by: Rusty Russell --- state.c | 19 ++++++------ state.h | 9 +++--- test/test_state_coverage.c | 59 ++++++++++++++++++++------------------ 3 files changed, 44 insertions(+), 43 deletions(-) diff --git a/state.c b/state.c index b04a3a2ce..43049c14f 100644 --- a/state.c +++ b/state.c @@ -633,8 +633,7 @@ enum command_status state(const tal_t *ctx, set_peer_cond(peer, PEER_CLOSED); return next_state(peer, cstatus, STATE_CLOSE_WAIT_CLOSE); } else if (input_is(input, PKT_ERROR)) { - add_effect(effect, in_error, - tal_steal(ctx, idata->pkt)); + peer_unexpected_pkt(peer, idata->pkt); goto start_unilateral_close_already_closing; } else if (input_is_pkt(input)) { /* We ignore all other packets while closing. */ @@ -656,12 +655,11 @@ enum command_status state(const tal_t *ctx, /* Just wait for close to happen now. */ return next_state(peer, cstatus, STATE_CLOSE_WAIT_CLOSE); } else if (input_is_pkt(input)) { - if (input_is(input, PKT_ERROR)) { - add_effect(effect, in_error, - tal_steal(ctx, idata->pkt)); - } else { + peer_unexpected_pkt(peer, idata->pkt); + /* Don't reply to an error with an error. */ + if (!input_is(input, PKT_ERROR)) { add_effect(effect, send_pkt, - unexpected_pkt(ctx, input)); + pkt_err_unexpected(ctx, idata->pkt)); } set_peer_cond(peer, PEER_CLOSED); /* Just wait for close to happen now. */ @@ -926,12 +924,13 @@ unexpected_pkt: /* * We got a weird packet, so we need to close unilaterally. */ + peer_unexpected_pkt(peer, idata->pkt); + /* Don't reply to an error with an error. */ if (input_is(input, PKT_ERROR)) { - add_effect(effect, in_error, tal_steal(ctx, idata->pkt)); goto start_unilateral_close; } - err = unexpected_pkt(ctx, input); + err = pkt_err_unexpected(ctx, idata->pkt); goto err_start_unilateral_close; unexpected_pkt_nocleanup: @@ -942,7 +941,7 @@ unexpected_pkt_nocleanup: if (input_is(input, PKT_ERROR)) { goto close_nocleanup; } - err = unexpected_pkt(ctx, input); + err = pkt_err_unexpected(ctx, idata->pkt); goto err_close_nocleanup; anchor_unspent: diff --git a/state.h b/state.h index d0956a22b..ef54f5ecc 100644 --- a/state.h +++ b/state.h @@ -18,7 +18,6 @@ enum state_peercond { }; enum state_effect_type { - STATE_EFFECT_in_error, STATE_EFFECT_broadcast_tx, STATE_EFFECT_send_pkt, STATE_EFFECT_watch, @@ -63,9 +62,6 @@ struct state_effect { /* Set a timeout for close tx. */ enum state_input close_timeout; - /* Error received from other side. */ - Pkt *in_error; - /* HTLC we're working on. */ struct htlc_progress *htlc_in_progress; @@ -155,6 +151,9 @@ static inline bool input_is(enum state_input a, enum state_input b) struct signature; +/* Inform peer have an unexpected packet. */ +void peer_unexpected_pkt(struct peer *peer, const Pkt *pkt); + /* Create various kinds of packets, allocated off @ctx */ Pkt *pkt_open(const tal_t *ctx, const struct peer *peer, OpenChannel__AnchorOffer anchor); @@ -176,7 +175,7 @@ Pkt *pkt_err(const tal_t *ctx, const char *msg); Pkt *pkt_close(const tal_t *ctx, const struct peer *peer); Pkt *pkt_close_complete(const tal_t *ctx, const struct peer *peer); Pkt *pkt_close_ack(const tal_t *ctx, const struct peer *peer); -Pkt *unexpected_pkt(const tal_t *ctx, enum state_input input); +Pkt *pkt_err_unexpected(const tal_t *ctx, const Pkt *pkt); /* Process various packets: return an error packet on failure. */ Pkt *accept_pkt_open(const tal_t *ctx, diff --git a/test/test_state_coverage.c b/test/test_state_coverage.c index 3aaa5162f..b99679bb0 100644 --- a/test/test_state_coverage.c +++ b/test/test_state_coverage.c @@ -623,9 +623,10 @@ Pkt *pkt_close_ack(const tal_t *ctx, const struct peer *peer) return new_pkt(ctx, PKT_CLOSE_ACK); } -Pkt *unexpected_pkt(const tal_t *ctx, enum state_input input) +Pkt *pkt_err_unexpected(const tal_t *ctx, const Pkt *pkt) { - return pkt_err(ctx, "Unexpected pkt"); + return (Pkt *)tal_fmt(ctx, "PKT_ERROR: Unexpected pkt %s", + (const char *)pkt); } Pkt *accept_pkt_open(const tal_t *ctx, @@ -1367,24 +1368,20 @@ static bool rval_known(const struct peer *peer, unsigned int id) if (peer->rvals_known[i] == id) return true; return false; -} - -/* Some assertions once they've already been applied. */ -static char *check_effects(struct peer *peer, - const struct state_effect *effect) -{ - while (effect) { - if (effect->etype == STATE_EFFECT_in_error) { - /* We should stop talking to them after error recvd. */ - if (peer->cond != PEER_CLOSING - && peer->cond != PEER_CLOSED) - return "packets still open after error pkt"; - } - effect = effect->next; - } - return NULL; } - + +void peer_unexpected_pkt(struct peer *peer, const Pkt *pkt) +{ + const char *str = (const char *)pkt; + + /* We can get errors. */ + if (strstarts(str, "PKT_ERROR:")) + return; + + /* Shouldn't get any other unexpected packets. */ + report_trail(peer->trail, "Unexpected packet"); +} + /* We apply them backwards, which helps our assertions. It's not actually * required. */ static const char *apply_effects(struct peer *peer, @@ -1409,8 +1406,6 @@ static const char *apply_effects(struct peer *peer, *effects |= (1ULL << effect->etype); switch (effect->etype) { - case STATE_EFFECT_in_error: - break; case STATE_EFFECT_broadcast_tx: break; case STATE_EFFECT_send_pkt: { @@ -1621,7 +1616,8 @@ static const char *apply_effects(struct peer *peer, return NULL; } -static const char *check_changes(const struct peer *old, struct peer *new) +static const char *check_changes(const struct peer *old, struct peer *new, + enum state_input input) { if (new->cond != old->cond) { /* Only BUSY -> CMD_OK can go backwards. */ @@ -1643,12 +1639,19 @@ static const char *check_changes(const struct peer *old, struct peer *new) remove_event_(&new->core.event_notifies, INPUT_CLOSE_COMPLETE_TIMEOUT); } - + + if (input == PKT_ERROR) { + /* We should stop talking to them after error recvd. */ + if (new->cond != PEER_CLOSING + && new->cond != PEER_CLOSED) + return "packets still open after error pkt"; + } return NULL; } static const char *apply_all_effects(const struct peer *old, enum command_status cstatus, + enum state_input input, struct peer *peer, const struct state_effect *effect, Pkt **output) @@ -1669,9 +1672,7 @@ static const char *apply_all_effects(const struct peer *old, problem = apply_effects(peer, effect, &effects, output); if (!problem) - problem = check_effects(peer, effect); - if (!problem) - problem = check_changes(old, peer); + problem = check_changes(old, peer, input); return problem; } @@ -1928,7 +1929,7 @@ static void try_input(const struct peer *peer, get_send_pkt(effect)); } - problem = apply_all_effects(peer, cstatus, ©, effect, &output); + problem = apply_all_effects(peer, cstatus, i, ©, effect, &output); update_trail(&t, ©, output); if (problem) report_trail(&t, problem); @@ -2242,7 +2243,9 @@ static void run_peer(const struct peer *peer, if (other.core.num_outputs) { i = other.core.outputs[0]; - if (other.pkt_data[0] == -1U) + if (i == PKT_ERROR) + idata->pkt = pkt_err(idata, ""); + else if (other.pkt_data[0] == -1U) idata->pkt = (Pkt *)talz(idata, char); else idata->pkt = htlc_pkt(idata, input_name(i),