Skip to content

Commit a394061

Browse files
capflamcapflam
committed
BUG/MEDIUM: mux-spop: Remove frame parsing states from the SPOP connection state
SPOP_CS_FRAME_H and SPOP_CS_FRAME_P states, that were used to handle frame parsing, were removed. The demux process now relies on the demux stream ID to know if it is waiting for the frame header or the frame payload. Concretly, when the demux stream ID is not set (dsi == -1), the demuxer is waiting for the next frame header. Otherwise (dsi >= 0), it is waiting for the frame payload. It is especially important to be able to properly handle DISCONNECT frames sent by the agents. SPOP_CS_RUNNING state is introduced to know the hello handshake was finished and the SPOP connection is able to open SPOP streams and exchange NOTIFY/ACK frames with the agents. It depends on the following fixes: * MINOR: mux-spop: Don't set SPOP connection state to FRAME_H after ACK parsing * BUG/MINOR: mux-spop: Make the demux stream ID a signed integer This change will be mandatory for the next fix. It must be backported to 3.1 with the commits above.
1 parent 6b0f7de commit a394061

File tree

2 files changed

+22
-24
lines changed

2 files changed

+22
-24
lines changed

‎include/haproxy/mux_spop-t.h‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,9 @@ static forceinline char *spop_strm_show_flags(char *buf, size_t len, const char
8787

8888
/* SPOP connection state (spop_conn->state) */
8989
enum spop_conn_st {
90-
SPOP_CS_HA_HELLO = 0, /* init done, waiting for sending HELLO frame */
91-
SPOP_CS_AGENT_HELLO, /* HELLO frame sent, waiting for agent HELLO frame to define the connection settings */
92-
SPOP_CS_FRAME_H, /* HELLO handshake finished, waiting for a frame header */
93-
SPOP_CS_FRAME_P, /* Frame header received, waiting for a frame data */
90+
SPOP_CS_HA_HELLO = 0, /* init done, waiting for sending HELLO frame */
91+
SPOP_CS_AGENT_HELLO, /* HELLO frame sent, waiting for agent HELLO frame to define the connection settings */
92+
SPOP_CS_RUNNING, /* HELLO handshake finished, exchange NOTIFY/ACK frames */
9493
SPOP_CS_ERROR, /* send DISCONNECT frame to be able ti close the connection */
9594
SPOP_CS_CLOSING, /* DISCONNECT frame sent, waiting for the agent DISCONNECT frame before closing */
9695
SPOP_CS_CLOSED, /* Agent DISCONNECT frame received and close the connection ASAP */
@@ -103,8 +102,7 @@ static inline const char *spop_conn_st_to_str(enum spop_conn_st st)
103102
switch (st) {
104103
case SPOP_CS_HA_HELLO : return "HHL";
105104
case SPOP_CS_AGENT_HELLO: return "AHL";
106-
case SPOP_CS_FRAME_H : return "FRH";
107-
case SPOP_CS_FRAME_P : return "FRP";
105+
case SPOP_CS_RUNNING : return "RUN";
108106
case SPOP_CS_ERROR : return "ERR";
109107
case SPOP_CS_CLOSING : return "CLI";
110108
case SPOP_CS_CLOSED : return "CLO";

‎src/mux_spop.c‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -834,7 +834,7 @@ static inline int spop_conn_is_dead(struct spop_conn *spop_conn)
834834
{
835835
if (eb_is_empty(&spop_conn->streams_by_id) && /* don't close if streams exist */
836836
((spop_conn->flags & SPOP_CF_ERROR) || /* errors close immediately */
837-
(spop_conn->flags & SPOP_CF_ERR_PENDING && spop_conn->state < SPOP_CS_FRAME_H) || /* early error during connect */
837+
(spop_conn->flags & SPOP_CF_ERR_PENDING && spop_conn->state < SPOP_CS_RUNNING) || /* early error during connect */
838838
(spop_conn->state == SPOP_CS_CLOSED && !spop_conn->task) ||/* a timeout stroke earlier */
839839
(!(spop_conn->conn->owner)) || /* Nobody's left to take care of the connection, drop it now */
840840
(!br_data(spop_conn->mbuf) && /* mux buffer empty, also process clean events below */
@@ -1782,6 +1782,7 @@ static int spop_conn_handle_hello(struct spop_conn *spop_conn)
17821782
TRACE_PROTO("SPOP AGENT HELLO frame rcvd", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn, 0, 0, (size_t[]){spop_conn->dfl});
17831783
b_del(&spop_conn->dbuf, spop_conn->dfl);
17841784
spop_conn->dfl = 0;
1785+
spop_conn->state = SPOP_CS_RUNNING;
17851786
spop_wake_unassigned_streams(spop_conn);
17861787
TRACE_LEAVE(SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn);
17871788
return 1;
@@ -2064,7 +2065,7 @@ static void spop_process_demux(struct spop_conn *spop_conn)
20642065
if (spop_conn->state >= SPOP_CS_ERROR)
20652066
goto out;
20662067

2067-
if (unlikely(spop_conn->state < SPOP_CS_FRAME_H)) {
2068+
if (unlikely(spop_conn->state < SPOP_CS_RUNNING)) {
20682069
if (spop_conn->state == SPOP_CS_HA_HELLO) {
20692070
TRACE_STATE("waiting AGENT HELLO frame to be sent", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO, spop_conn->conn);
20702071
goto out;
@@ -2113,7 +2114,7 @@ static void spop_process_demux(struct spop_conn *spop_conn)
21132114
break;
21142115
}
21152116

2116-
if (spop_conn->state == SPOP_CS_FRAME_H) {
2117+
if (spop_conn->dsi == -1) {
21172118
TRACE_PROTO("receiving SPOP frame header", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
21182119
if (!spop_get_frame_hdr(&spop_conn->dbuf, &hdr)) {
21192120
spop_conn->flags |= SPOP_CF_DEM_SHORT_READ;
@@ -2133,8 +2134,7 @@ static void spop_process_demux(struct spop_conn *spop_conn)
21332134
spop_conn->dft = hdr.type;
21342135
spop_conn->dfl = hdr.len;
21352136
spop_conn->dff = hdr.flags;
2136-
spop_conn->state = SPOP_CS_FRAME_P;
2137-
TRACE_STATE("SPOP frame header rcvd, switching to FRAME_P", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
2137+
TRACE_STATE("SPOP frame header rcvd", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
21382138

21392139
/* Perform sanity check on the frame header */
21402140
if (!(spop_conn->dff & SPOP_FRM_FL_FIN)) {
@@ -2211,15 +2211,15 @@ static void spop_process_demux(struct spop_conn *spop_conn)
22112211

22122212
switch (spop_conn->dft) {
22132213
case SPOP_FRM_T_AGENT_HELLO:
2214-
if (spop_conn->state == SPOP_CS_FRAME_P)
2214+
if (spop_conn->dsi == 0)
22152215
ret = spop_conn_handle_hello(spop_conn);
22162216
break;
22172217
case SPOP_FRM_T_AGENT_DISCON:
2218-
if (spop_conn->state == SPOP_CS_FRAME_P)
2218+
if (spop_conn->dsi == 0)
22192219
ret = spop_conn_handle_disconnect(spop_conn);
22202220
break;
22212221
case SPOP_FRM_T_AGENT_ACK:
2222-
if (spop_conn->state == SPOP_CS_FRAME_P)
2222+
if (spop_conn->dsi > 0)
22232223
ret = spop_conn_handle_ack(spop_conn, spop_strm);
22242224
break;
22252225
default:
@@ -2244,16 +2244,16 @@ static void spop_process_demux(struct spop_conn *spop_conn)
22442244
if (ret <= 0)
22452245
break;
22462246

2247-
if (spop_conn->state != SPOP_CS_FRAME_H) {
2247+
if (spop_conn->dsi >= 0) {
22482248
if (spop_conn->dfl) {
22492249
TRACE_DEVEL("skipping remaining frame payload", SPOP_EV_RX_FRAME, spop_conn->conn, spop_strm);
22502250
ret = MIN(b_data(&spop_conn->dbuf), spop_conn->dfl);
22512251
b_del(&spop_conn->dbuf, ret);
22522252
spop_conn->dfl -= ret;
22532253
}
22542254
if (!spop_conn->dfl) {
2255-
TRACE_STATE("switching to FRAME_H", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
2256-
spop_conn->state = SPOP_CS_FRAME_H;
2255+
TRACE_STATE("reset dsi", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
2256+
spop_conn->dsi = -1;
22572257
}
22582258
}
22592259
}
@@ -2265,11 +2265,11 @@ static void spop_process_demux(struct spop_conn *spop_conn)
22652265
}
22662266

22672267
if (spop_conn->flags & SPOP_CF_ERROR)
2268-
spop_conn_report_term_evt(spop_conn, ((eb_is_empty(&spop_conn->streams_by_id) && (spop_conn->state == SPOP_CS_FRAME_H))
2268+
spop_conn_report_term_evt(spop_conn, ((eb_is_empty(&spop_conn->streams_by_id) && (spop_conn->state == SPOP_CS_RUNNING) && spop_conn->dsi == -1)
22692269
? muxc_tevt_type_rcv_err
22702270
: muxc_tevt_type_truncated_rcv_err));
22712271
else if (spop_conn->flags & SPOP_CF_END_REACHED)
2272-
spop_conn_report_term_evt(spop_conn, ((eb_is_empty(&spop_conn->streams_by_id) && (spop_conn->state == SPOP_CS_FRAME_H))
2272+
spop_conn_report_term_evt(spop_conn, ((eb_is_empty(&spop_conn->streams_by_id) && (spop_conn->state == SPOP_CS_RUNNING) && spop_conn->dsi == -1)
22732273
? muxc_tevt_type_shutr
22742274
: muxc_tevt_type_truncated_shutr));
22752275

@@ -2299,7 +2299,7 @@ static int spop_process_mux(struct spop_conn *spop_conn)
22992299
{
23002300
TRACE_ENTER(SPOP_EV_SPOP_CONN_WAKE, spop_conn->conn);
23012301

2302-
if (unlikely(spop_conn->state < SPOP_CS_FRAME_H)) {
2302+
if (unlikely(spop_conn->state < SPOP_CS_RUNNING)) {
23032303
if (unlikely(spop_conn->state == SPOP_CS_HA_HELLO)) {
23042304
TRACE_PROTO("sending SPOP HAPROXY HELLO fraame", SPOP_EV_TX_FRAME, spop_conn->conn);
23052305
if (unlikely(!spop_conn_send_hello(spop_conn)))
@@ -2308,7 +2308,7 @@ static int spop_process_mux(struct spop_conn *spop_conn)
23082308
TRACE_STATE("waiting for SPOP AGENT HELLO reply", SPOP_EV_TX_FRAME|SPOP_EV_RX_FRAME, spop_conn->conn);
23092309
}
23102310
/* need to wait for the other side */
2311-
if (spop_conn->state < SPOP_CS_FRAME_H)
2311+
if (spop_conn->state < SPOP_CS_RUNNING)
23122312
goto done;
23132313
}
23142314

@@ -2490,7 +2490,7 @@ static int spop_send(struct spop_conn *spop_conn)
24902490
/* We're not full anymore, so we can wake any task that are waiting
24912491
* for us.
24922492
*/
2493-
if (!(spop_conn->flags & (SPOP_CF_MUX_MFULL | SPOP_CF_DEM_MROOM)) && spop_conn->state >= SPOP_CS_FRAME_H) {
2493+
if (!(spop_conn->flags & (SPOP_CF_MUX_MFULL | SPOP_CF_DEM_MROOM)) && spop_conn->state >= SPOP_CS_RUNNING) {
24942494
spop_conn->flags &= ~SPOP_CF_WAIT_INLIST;
24952495
spop_resume_each_sending_spop_strm(spop_conn, &spop_conn->send_list);
24962496
}
@@ -2665,7 +2665,7 @@ static int spop_ctl(struct connection *conn, enum mux_ctl_type mux_ctl, void *ou
26652665

26662666
switch (mux_ctl) {
26672667
case MUX_CTL_STATUS:
2668-
if ((spop_conn->state >= SPOP_CS_FRAME_H && spop_conn->state < SPOP_CS_ERROR) &&
2668+
if (spop_conn->state == SPOP_CS_RUNNING &&
26692669
!(spop_conn->flags & (SPOP_CF_ERROR|SPOP_CF_ERR_PENDING|SPOP_CF_END_REACHED|SPOP_CF_RCVD_SHUT|SPOP_CF_DISCO_SENT|SPOP_CF_DISCO_FAILED)))
26702670
ret |= MUX_STATUS_READY;
26712671
return ret;
@@ -3328,7 +3328,7 @@ static size_t spop_snd_buf(struct stconn *sc, struct buffer *buf, size_t count,
33283328
}
33293329
spop_strm->flags &= ~SPOP_SF_NOTIFIED;
33303330

3331-
if (spop_conn->state < SPOP_CS_FRAME_H) {
3331+
if (spop_conn->state < SPOP_CS_RUNNING) {
33323332
TRACE_DEVEL("connection not ready, leaving", SPOP_EV_STRM_SEND|SPOP_EV_SPOP_STRM_BLK, spop_conn->conn, spop_strm);
33333333
return 0;
33343334
}

0 commit comments

Comments
 (0)