From af642df3b843e94c0a96d88d862e2b8ca6b44d3b Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Wed, 30 Mar 2022 10:06:11 +0200 Subject: [PATCH] MINOR: stream-int/conn-stream: Report error to the CS instead of the SI SI_FL_ERR is removed and replaced by CS_FL_ERROR. It is a transient patch because the idea is to rely on the endpoint to handle errors at this level. But if for any reason it is not possible, the stream-interface flags will still be replaced. --- dev/flags/flags.c | 2 +- include/haproxy/conn_stream-t.h | 2 ++ include/haproxy/cs_utils.h | 10 ++++++++ include/haproxy/stream_interface-t.h | 3 +-- include/haproxy/stream_interface.h | 2 +- src/backend.c | 20 +++++++-------- src/cli.c | 2 +- src/http_ana.c | 3 ++- src/stream.c | 24 +++++++++--------- src/stream_interface.c | 38 ++++++++++------------------ 10 files changed, 54 insertions(+), 52 deletions(-) diff --git a/dev/flags/flags.c b/dev/flags/flags.c index f43fe3a6c..c38d90efe 100644 --- a/dev/flags/flags.c +++ b/dev/flags/flags.c @@ -219,6 +219,7 @@ void show_cs_flags(unsigned int f) printf("0\n"); return; } + SHOW_FLAG(f, CS_FL_ERR); SHOW_FLAG(f, CS_FL_ADDR_FROM_SET); SHOW_FLAG(f, CS_FL_ADDR_TO_SET); SHOW_FLAG(f, CS_FL_ISBACK); @@ -263,7 +264,6 @@ void show_si_flags(unsigned int f) return; } - SHOW_FLAG(f, SI_FL_ERR); SHOW_FLAG(f, SI_FL_KILL_CONN); SHOW_FLAG(f, SI_FL_WAIT_DATA); SHOW_FLAG(f, SI_FL_ISBACK); diff --git a/include/haproxy/conn_stream-t.h b/include/haproxy/conn_stream-t.h index 55f338b09..8530586cd 100644 --- a/include/haproxy/conn_stream-t.h +++ b/include/haproxy/conn_stream-t.h @@ -82,6 +82,8 @@ enum { CS_FL_ADDR_FROM_SET = 0x00000002, /* source address is set */ CS_FL_ADDR_TO_SET = 0x00000004, /* destination address is set */ + + CS_FL_ERR = 0x00000008, /* a non-recoverable error has occurred */ }; /* cs_shutr() modes */ diff --git a/include/haproxy/cs_utils.h b/include/haproxy/cs_utils.h index 7381499dc..2346a8577 100644 --- a/include/haproxy/cs_utils.h +++ b/include/haproxy/cs_utils.h @@ -76,6 +76,16 @@ static inline struct conn_stream *cs_opposite(struct conn_stream *cs) } +/* to be called only when in SI_ST_DIS with SI_FL_ERR */ +static inline void cs_report_error(struct conn_stream *cs) +{ + if (!cs->si->err_type) + cs->si->err_type = SI_ET_DATA_ERR; + + cs_oc(cs)->flags |= CF_WRITE_ERROR; + cs_ic(cs)->flags |= CF_READ_ERROR; +} + /* Returns the source address of the conn-stream and, if not set, fallbacks on * the session for frontend CS and the server connection for the backend CS. It * returns a const address on success or NULL on failure. diff --git a/include/haproxy/stream_interface-t.h b/include/haproxy/stream_interface-t.h index 50b75d7d5..5fe805ed1 100644 --- a/include/haproxy/stream_interface-t.h +++ b/include/haproxy/stream_interface-t.h @@ -83,8 +83,7 @@ enum { /* flags set after I/O (32 bit) */ enum { SI_FL_NONE = 0x00000000, /* nothing */ - /* unused: 0x00000001 */ - SI_FL_ERR = 0x00000002, /* a non-recoverable error has occurred */ + /* unused: 0x00000001, 0x00000002 */ SI_FL_KILL_CONN = 0x00000004, /* next shutw must kill the whole conn, not just the stream */ SI_FL_WAIT_DATA = 0x00000008, /* stream-int waits for more outgoing data to send */ SI_FL_ISBACK = 0x00000010, /* 0 for front-side SI, 1 for back-side */ diff --git a/include/haproxy/stream_interface.h b/include/haproxy/stream_interface.h index 6372c1a30..4424a7e7a 100644 --- a/include/haproxy/stream_interface.h +++ b/include/haproxy/stream_interface.h @@ -39,7 +39,7 @@ struct stream_interface *si_new(struct conn_stream *cs); void si_free(struct stream_interface *si); /* main event functions used to move data between sockets and buffers */ -void si_report_error(struct stream_interface *si); + void si_retnclose(struct stream_interface *si, const struct buffer *msg); int conn_si_send_proxy(struct connection *conn, unsigned int flag); struct appctx *si_register_handler(struct stream_interface *si, struct applet *app); diff --git a/src/backend.c b/src/backend.c index 2d7d91d8e..7250aa9a8 100644 --- a/src/backend.c +++ b/src/backend.c @@ -1741,11 +1741,11 @@ static int connect_server(struct stream *s) /* Now handle synchronously connected sockets. We know the stream-int * is at least in state SI_ST_CON. These ones typically are UNIX - * sockets, socket pairs, and occasionally TCP connections on the + * sockets, socket pairs, andoccasionally TCP connections on the * loopback on a heavily loaded system. */ if ((srv_conn->flags & CO_FL_ERROR || s->csb->endp->flags & CS_EP_ERROR)) - cs_si(s->csb)->flags |= SI_FL_ERR; + s->csb->flags |= CS_FL_ERR; /* If we had early data, and the handshake ended, then * we can remove the flag, and attempt to wake the task up, @@ -1964,7 +1964,7 @@ void back_try_conn_req(struct stream *s) * allocation problem, so we want to retry now. */ cs->si->state = SI_ST_CER; - cs->si->flags &= ~SI_FL_ERR; + cs->flags &= ~CS_FL_ERR; back_handle_st_cer(s); DBG_TRACE_STATE("connection error, retry", STRM_EV_STRM_PROC|STRM_EV_SI_ST|STRM_EV_STRM_ERR, s); @@ -2187,9 +2187,9 @@ void back_handle_st_con(struct stream *s) done: /* retryable error ? */ - if ((s->flags & SF_CONN_EXP) || (cs->si->flags & SI_FL_ERR)) { + if ((s->flags & SF_CONN_EXP) || (cs->flags & CS_FL_ERR)) { if (!cs->si->err_type) { - if (cs->si->flags & SI_FL_ERR) + if (cs->flags & CS_FL_ERR) cs->si->err_type = SI_ET_CONN_ERR; else cs->si->err_type = SI_ET_CONN_TO; @@ -2234,7 +2234,7 @@ void back_handle_st_cer(struct stream *s) _HA_ATOMIC_DEC(&__objt_server(s->target)->cur_sess); } - if ((cs->si->flags & SI_FL_ERR) && + if ((cs->flags & CS_FL_ERR) && conn && conn->err_code == CO_ER_SSL_MISMATCH_SNI) { /* We tried to connect to a server which is configured * with "verify required" and which doesn't have the @@ -2291,7 +2291,7 @@ void back_handle_st_cer(struct stream *s) * layers in an unexpected state (i.e < ST_CONN). * * Note: the stream-interface will be switched to ST_REQ, ST_ASS or - * ST_TAR and SI_FL_ERR and SF_CONN_EXP flags will be unset. + * ST_TAR and CS_FL_ERR and SF_CONN_EXP flags will be unset. */ if (cs_reset_endp(cs) < 0) { if (!cs->si->err_type) @@ -2319,7 +2319,7 @@ void back_handle_st_cer(struct stream *s) stream_choose_redispatch(s); - if (cs->si->flags & SI_FL_ERR) { + if (cs->flags & CS_FL_ERR) { /* The error was an asynchronous connection error, and we will * likely have to retry connecting to the same server, most * likely leading to the same result. To avoid this, we wait @@ -2345,7 +2345,7 @@ void back_handle_st_cer(struct stream *s) cs->si->state = SI_ST_TAR; s->conn_exp = tick_add(now_ms, MS_TO_TICKS(delay)); } - cs->si->flags &= ~SI_FL_ERR; + cs->flags &= ~CS_FL_ERR; DBG_TRACE_STATE("retry a new connection", STRM_EV_STRM_PROC|STRM_EV_SI_ST, s); } @@ -2401,7 +2401,7 @@ void back_handle_st_rdy(struct stream *s) } /* retryable error ? */ - if (cs->si->flags & SI_FL_ERR) { + if (cs->flags & CS_FL_ERR) { if (!cs->si->err_type) cs->si->err_type = SI_ET_CONN_ERR; cs->si->state = SI_ST_CER; diff --git a/src/cli.c b/src/cli.c index b5330db19..35d2d36dc 100644 --- a/src/cli.c +++ b/src/cli.c @@ -1084,7 +1084,7 @@ static void cli_io_handler(struct appctx *appctx) } break; default: /* abnormal state */ - cs->si->flags |= SI_FL_ERR; + cs->flags |= CS_FL_ERR; break; } diff --git a/src/http_ana.c b/src/http_ana.c index f87db34c6..927f0b96c 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -1247,8 +1247,9 @@ static __inline int do_l7_retry(struct stream *s, struct stream_interface *si) req->flags &= ~(CF_WRITE_ERROR | CF_WRITE_TIMEOUT | CF_SHUTW | CF_SHUTW_NOW); res->flags &= ~(CF_READ_ERROR | CF_READ_TIMEOUT | CF_SHUTR | CF_EOI | CF_READ_NULL | CF_SHUTR_NOW); res->analysers &= AN_RES_FLT_END; - si->flags &= ~(SI_FL_ERR | SI_FL_RXBLK_SHUT); + si->flags &= ~SI_FL_RXBLK_SHUT; si->err_type = SI_ET_NONE; + si->cs->flags &= ~CS_FL_ERR; s->flags &= ~(SF_CONN_EXP | SF_ERR_MASK | SF_FINST_MASK); s->conn_exp = TICK_ETERNITY; stream_choose_redispatch(s); diff --git a/src/stream.c b/src/stream.c index c80f63c92..e441b6ed2 100644 --- a/src/stream.c +++ b/src/stream.c @@ -880,7 +880,7 @@ static void back_establish(struct stream *s) s->flags &= ~SF_CONN_EXP; /* errors faced after sending data need to be reported */ - if (si->flags & SI_FL_ERR && req->flags & CF_WROTE_DATA) { + if (si->cs->flags & CS_FL_ERR && req->flags & CF_WROTE_DATA) { /* Don't add CF_WRITE_ERROR if we're here because * early data were rejected by the server, or * http_wait_for_response() will never be called @@ -1673,7 +1673,7 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) (CF_SHUTR|CF_READ_ACTIVITY|CF_READ_TIMEOUT|CF_SHUTW| CF_WRITE_ACTIVITY|CF_WRITE_TIMEOUT|CF_ANA_TIMEOUT)) && !(s->flags & SF_CONN_EXP) && - !((si_f->flags | si_b->flags) & SI_FL_ERR) && + !((si_f->cs->flags | si_b->cs->flags) & CS_FL_ERR) && ((s->pending_events & TASK_WOKEN_ANY) == TASK_WOKEN_TIMER)) { si_f->flags &= ~SI_FL_DONT_WAKE; si_b->flags &= ~SI_FL_DONT_WAKE; @@ -1692,10 +1692,10 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) * must be be reviewed too. */ if (!stream_alloc_work_buffer(s)) { - si_f->flags |= SI_FL_ERR; + si_f->cs->flags |= CS_FL_ERR; si_f->err_type = SI_ET_CONN_RES; - si_b->flags |= SI_FL_ERR; + si_b->cs->flags |= CS_FL_ERR; si_b->err_type = SI_ET_CONN_RES; if (!(s->flags & SF_ERR_MASK)) @@ -1711,11 +1711,11 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) * connection setup code must be able to deal with any type of abort. */ srv = objt_server(s->target); - if (unlikely(si_f->flags & SI_FL_ERR)) { + if (unlikely(si_f->cs->flags & CS_FL_ERR)) { if (si_state_in(si_f->state, SI_SB_EST|SI_SB_DIS)) { si_shutr(si_f); si_shutw(si_f); - si_report_error(si_f); + cs_report_error(si_f->cs); if (!(req->analysers) && !(res->analysers)) { _HA_ATOMIC_INC(&s->be->be_counters.cli_aborts); _HA_ATOMIC_INC(&sess->fe->fe_counters.cli_aborts); @@ -1731,11 +1731,11 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) } } - if (unlikely(si_b->flags & SI_FL_ERR)) { + if (unlikely(si_b->cs->flags & CS_FL_ERR)) { if (si_state_in(si_b->state, SI_SB_EST|SI_SB_DIS)) { si_shutr(si_b); si_shutw(si_b); - si_report_error(si_b); + cs_report_error(si_b->cs); _HA_ATOMIC_INC(&s->be->be_counters.failed_resp); if (srv) _HA_ATOMIC_INC(&srv->counters.failed_resp); @@ -2255,8 +2255,8 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) /* Benchmarks have shown that it's optimal to do a full resync now */ if (si_f->state == SI_ST_DIS || si_state_in(si_b->state, SI_SB_RDY|SI_SB_DIS) || - (si_f->flags & SI_FL_ERR && si_f->state != SI_ST_CLO) || - (si_b->flags & SI_FL_ERR && si_b->state != SI_ST_CLO)) + (si_f->cs->flags & CS_FL_ERR && si_f->state != SI_ST_CLO) || + (si_b->cs->flags & CS_FL_ERR && si_b->state != SI_ST_CLO)) goto resync_stream_interface; /* otherwise we want to check if we need to resync the req buffer or not */ @@ -2379,8 +2379,8 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) if (si_f->state == SI_ST_DIS || si_state_in(si_b->state, SI_SB_RDY|SI_SB_DIS) || - (si_f->flags & SI_FL_ERR && si_f->state != SI_ST_CLO) || - (si_b->flags & SI_FL_ERR && si_b->state != SI_ST_CLO)) + (si_f->cs->flags & CS_FL_ERR && si_f->state != SI_ST_CLO) || + (si_b->cs->flags & CS_FL_ERR && si_b->state != SI_ST_CLO)) goto resync_stream_interface; if ((req->flags & ~rqf_last) & CF_MASK_ANALYSER) diff --git a/src/stream_interface.c b/src/stream_interface.c index 001a34d08..395fcf818 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -128,16 +128,6 @@ void si_free(struct stream_interface *si) pool_free(pool_head_streaminterface, si); } -/* to be called only when in SI_ST_DIS with SI_FL_ERR */ -void si_report_error(struct stream_interface *si) -{ - if (!si->err_type) - si->err_type = SI_ET_DATA_ERR; - - si_oc(si)->flags |= CF_WRITE_ERROR; - si_ic(si)->flags |= CF_READ_ERROR; -} - /* * Returns a message to the client ; the connection is shut down for read, * and the request is cleared so that no server connection can be initiated. @@ -235,7 +225,7 @@ static void stream_int_shutw(struct stream_interface *si) * However, if SI_FL_NOLINGER is explicitly set, we know there is * no risk so we close both sides immediately. */ - if (!(si->flags & (SI_FL_ERR | SI_FL_NOLINGER)) && + if (!(si->cs->flags & CS_FL_ERR) && !(si->flags & SI_FL_NOLINGER) && !(ic->flags & (CF_SHUTR|CF_DONT_READ))) return; @@ -531,7 +521,7 @@ static void stream_int_notify(struct stream_interface *si) if (/* changes on the production side */ (ic->flags & (CF_READ_NULL|CF_READ_ERROR)) || !si_state_in(si->state, SI_SB_CON|SI_SB_RDY|SI_SB_EST) || - (si->flags & SI_FL_ERR) || + (si->cs->flags & CS_FL_ERR) || ((ic->flags & CF_READ_PARTIAL) && ((ic->flags & CF_EOI) || !ic->to_forward || sio->state != SI_ST_EST)) || @@ -598,13 +588,13 @@ static int si_cs_process(struct conn_stream *cs) if (!channel_is_empty(oc) && !(si->wait_event.events & SUB_RETRY_SEND)) si_cs_send(cs); - /* First step, report to the stream-int what was detected at the + /* First step, report to the conn-stream what was detected at the * connection layer : errors and connection establishment. - * Only add SI_FL_ERR if we're connected, or we're attempting to + * Only add CS_FL_ERR if we're connected, or we're attempting to * connect, we may get there because we got woken up, but only run * after process_stream() noticed there were an error, and decided * to retry to connect, the connection may still have CO_FL_ERROR, - * and we don't want to add SI_FL_ERR back + * and we don't want to add CS_FL_ERR back * * Note: This test is only required because si_cs_process is also the SI * wake callback. Otherwise si_cs_recv()/si_cs_send() already take @@ -613,7 +603,7 @@ static int si_cs_process(struct conn_stream *cs) if (si->state >= SI_ST_CON) { if ((cs->endp->flags & CS_EP_ERROR) || si_is_conn_error(si)) - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; } /* If we had early data, and the handshake ended, then @@ -689,11 +679,11 @@ static int si_cs_send(struct conn_stream *cs) * but process_stream() ran before, detected there were an * error and put the si back to SI_ST_TAR. There's still * CO_FL_ERROR on the connection but we don't want to add - * SI_FL_ERR back, so give up + * CS_FL_ERR back, so give up */ if (si->state < SI_ST_CON) return 0; - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; return 1; } @@ -807,7 +797,7 @@ static int si_cs_send(struct conn_stream *cs) } if (cs->endp->flags & (CS_EP_ERROR|CS_EP_ERR_PENDING)) { - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; return 1; } @@ -1102,7 +1092,7 @@ static void stream_int_shutw_conn(struct stream_interface *si) if (si->flags & SI_FL_KILL_CONN) cs->endp->flags |= CS_EP_KILL_CONN; - if (si->flags & SI_FL_ERR) { + if (si->cs->flags & CS_FL_ERR) { /* quick close, the socket is already shut anyway */ } else if (si->flags & SI_FL_NOLINGER) { @@ -1192,7 +1182,7 @@ static void stream_int_chk_snd_conn(struct stream_interface *si) if (cs->endp->flags & (CS_EP_ERROR|CS_EP_ERR_PENDING) || si_is_conn_error(si)) { /* Write error on the file descriptor */ if (si->state >= SI_ST_CON) - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; goto out_wakeup; } @@ -1569,7 +1559,7 @@ static int si_cs_recv(struct conn_stream *cs) } if (cs->endp->flags & CS_EP_ERROR) { - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; ret = 1; } else if (cs->endp->flags & CS_EP_EOS) { @@ -1656,7 +1646,7 @@ void si_applet_wake_cb(struct stream_interface *si) * broken pipe and it must be reported. */ if (!(si->flags & SI_FL_RX_WAIT_EP) && (ic->flags & CF_SHUTR)) - si->flags |= SI_FL_ERR; + si->cs->flags |= CS_FL_ERR; /* automatically mark the applet having data available if it reported * begin blocked by the channel. @@ -1752,7 +1742,7 @@ static void stream_int_shutw_applet(struct stream_interface *si) * However, if SI_FL_NOLINGER is explicitly set, we know there is * no risk so we close both sides immediately. */ - if (!(si->flags & (SI_FL_ERR | SI_FL_NOLINGER)) && + if (!(si->cs->flags & CS_FL_ERR) && !(si->flags & SI_FL_NOLINGER) && !(ic->flags & (CF_SHUTR|CF_DONT_READ))) return;