Revert "BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT...

This reverts commit 53c3046898633e56f74f7f05fb38cabeea1c87a1.

This patch introduced a regression leading to a loop on the frames
demultiplexing because a frame may be ignore but not consumed.

But outside this regression that can be fixed, there is a design issue that
was not totally fixed by the patch above. The SPOP connection state is mixed
with the status of the frames demultiplexer and this needlessly complexify
the connection management. Instead of fixing the fix, a better solution is
to revert it to work a a proper solution.

For the record, the idea is to deal with the spop connection state onlu
using 'state' field and to introduce a new field to handle the frames
demultiplexer state. This should ease the closing state management.

Another issue that must be fixed. We must take care to not abort a SPOP
stream when an error is detected on a SPOP connection or when the connection
is closed, if the ACK frame was already received for this stream. It is not
a common case, but it can be solved by saving the last known stream ID that
recieved a ACK.

This patch must be backported if the commit above is backported.

diff --git a/src/mux_spop.c b/src/mux_spop.c
index cb49f14a3616c75d711a0c1ae5ff4af41bdcd91a..2513c3078cea21bcb8b4f7141df8f7312c30deb2 100644 (file)
--- a/src/mux_spop.c
+++ b/src/mux_spop.c
@@ -605,7 +605,7 @@ static int spop_avail_streams(struct connection *conn)
        int ret1, ret2;
 
        /* Don't open new stream if the connection is closed */
-       if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR)))
+       if (spop_conn->state == SPOP_CS_CLOSED)
                return 0;
 
        /* May be negative if this setting has changed */
@@ -1291,8 +1291,7 @@ static void spop_strm_wake_one_stream(struct spop_strm *spop_strm)
                        spop_strm_close(spop_strm);
        }
 
-
-       if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) {
+       if (spop_conn->state == SPOP_CS_CLOSED || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) {
                se_fl_set_error(spop_strm->sd);
                spop_strm_propagate_term_flags(spop_conn, spop_strm);
                if (!spop_strm->sd->abort_info.info) {
@@ -1556,7 +1555,6 @@ static int spop_conn_send_disconnect(struct spop_conn *spop_conn)
        spop_set_frame_size(outbuf.area, outbuf.data - 4);
        b_add(mbuf, outbuf.data);
        spop_conn->flags |= SPOP_CF_DISCO_SENT;
-       spop_conn->state = SPOP_CS_CLOSING;
        ret = 1;
 
        switch (spop_conn->errcode) {
@@ -1784,11 +1782,12 @@ static int spop_conn_handle_hello(struct spop_conn *spop_conn)
        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});
        b_del(&spop_conn->dbuf, spop_conn->dfl);
        spop_conn->dfl = 0;
-       spop_conn->state = SPOP_CS_FRAME_H;
        spop_wake_unassigned_streams(spop_conn);
        TRACE_LEAVE(SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn);
        return 1;
   fail:
+       spop_conn->state = SPOP_CS_CLOSED;
+       TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn);
        TRACE_DEVEL("leaving on error", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
        return 0;
 }
@@ -1974,6 +1973,7 @@ static int spop_conn_handle_ack(struct spop_conn *spop_conn, struct spop_strm *s
 
        sent = b_xfer(rxbuf, dbuf, flen);
        BUG_ON(sent != flen);
+       /* b_del(&spop_conn->dbuf, sent); */
        spop_conn->dfl -= sent;
 
   end:
@@ -2062,35 +2062,27 @@ static void spop_process_demux(struct spop_conn *spop_conn)
 
        TRACE_ENTER(SPOP_EV_SPOP_CONN_WAKE, spop_conn->conn);
 
-       if (spop_conn->state == SPOP_CS_CLOSED)
+       if (spop_conn->state >= SPOP_CS_ERROR)
                goto out;
 
-       if (unlikely(spop_conn->state < SPOP_CS_FRAME_H || spop_conn->state == SPOP_CS_CLOSING)) {
+       if (unlikely(spop_conn->state < SPOP_CS_FRAME_H)) {
                if (spop_conn->state == SPOP_CS_HA_HELLO) {
                        TRACE_STATE("waiting AGENT HELLO frame to be sent", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO, spop_conn->conn);
                        goto out;
                }
-               else { /* AGENT_HELLO OR CLOSING */
-                       /* ensure that what is pending is a valid AGENT HELLO/DISCONNECT frame. */
-                       TRACE_STATE("receiving AGENT HELLO/DISCONNECT frame header", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
+               if (spop_conn->state == SPOP_CS_AGENT_HELLO) {
+                       /* ensure that what is pending is a valid AGENT HELLO frame. */
+                       TRACE_STATE("receiving AGENT HELLO frame header", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
                        if (!spop_get_frame_hdr(&spop_conn->dbuf, &hdr)) {
                                spop_conn->flags |= SPOP_CF_DEM_SHORT_READ;
                                TRACE_ERROR("header frame not available yet", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
                                goto done;
                        }
 
-                       if (spop_conn->state == SPOP_CS_AGENT_HELLO && hdr.type != SPOP_FRM_T_AGENT_HELLO) {
+                       if (hdr.sid || hdr.fid || hdr.type != SPOP_FRM_T_AGENT_HELLO || !(hdr.flags & SPOP_FRM_FL_FIN)) {
                                spop_conn_error(spop_conn, SPOP_ERR_INVALID);
                                spop_conn->state = SPOP_CS_CLOSED;
-                               TRACE_ERROR("unexpected frame type (AGENT HELLO expected)", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
-                               TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
-                               goto done;
-                       }
-
-                       if ((hdr.type == SPOP_FRM_T_AGENT_HELLO || hdr.type == SPOP_FRM_T_AGENT_DISCON) && (hdr.sid || hdr.fid || !(hdr.flags & SPOP_FRM_FL_FIN))) {
-                               spop_conn_error(spop_conn, SPOP_ERR_INVALID);
-                               spop_conn->state = SPOP_CS_CLOSED;
-                               TRACE_ERROR("unexpected frame meta-data", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
+                               TRACE_ERROR("unexpected frame type or flags", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
                                TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn);
                                goto done;
                        }
@@ -2136,14 +2128,13 @@ static void spop_process_demux(struct spop_conn *spop_conn)
                                break;
                        }
 
-                       spop_conn->state = SPOP_CS_FRAME_P;
-
                  new_frame:
                        spop_conn->dsi = hdr.sid;
                        spop_conn->dfi = hdr.fid;
                        spop_conn->dft = hdr.type;
                        spop_conn->dfl = hdr.len;
                        spop_conn->dff = hdr.flags;
+                       spop_conn->state = SPOP_CS_FRAME_P;
                        TRACE_STATE("SPOP frame header rcvd, switching to FRAME_P", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
 
                        /* Perform sanity check on the frame header */
@@ -2221,10 +2212,12 @@ static void spop_process_demux(struct spop_conn *spop_conn)
 
                switch (spop_conn->dft) {
                case SPOP_FRM_T_AGENT_HELLO:
-                       ret = spop_conn_handle_hello(spop_conn);
+                       if (spop_conn->state == SPOP_CS_FRAME_P)
+                               ret = spop_conn_handle_hello(spop_conn);
                        break;
                case SPOP_FRM_T_AGENT_DISCON:
-                       ret = spop_conn_handle_disconnect(spop_conn);
+                       if (spop_conn->state == SPOP_CS_FRAME_P)
+                               ret = spop_conn_handle_disconnect(spop_conn);
                        break;
                case SPOP_FRM_T_AGENT_ACK:
                        if (spop_conn->state == SPOP_CS_FRAME_P)
@@ -2259,7 +2252,7 @@ static void spop_process_demux(struct spop_conn *spop_conn)
                                b_del(&spop_conn->dbuf, ret);
                                spop_conn->dfl -= ret;
                        }
-                       if (!spop_conn->dfl && spop_conn->state != SPOP_CS_FRAME_P) {
+                       if (!spop_conn->dfl) {
                                TRACE_STATE("switching to FRAME_H", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn);
                                spop_conn->state = SPOP_CS_FRAME_H;
                        }
@@ -2597,7 +2590,7 @@ static int spop_process(struct spop_conn *spop_conn)
            (b_data(&spop_conn->dbuf) || (spop_conn->flags & SPOP_CF_RCVD_SHUT))) {
                spop_process_demux(spop_conn);
 
-               if (spop_conn->state == SPOP_CS_CLOSED || (spop_conn->flags & SPOP_CF_ERROR))
+               if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & SPOP_CF_ERROR))
                        b_reset(&spop_conn->dbuf);
 
                if (b_room(&spop_conn->dbuf))