MINOR: quic: do not remove qc_stream_desc automatically on ACK handling

qc_stream_desc_ack() is used to handle ACK received for STREAM frame. It
removes acknowledged data from their underlying buffer.

If all data were removed after ACK handling, qc_stream_desc instance
would automatically be freed at the end of qc_stream_desc_ack().
However, this renders the function complicated to use. Simplify this by
removing this automatic removal. Now, caller is responsible to check
after ACK handling if qc_stream_desc instance can be removed. This is
easily done using qc_stream_desc_done() helper.

diff --git a/include/haproxy/quic_stream.h b/include/haproxy/quic_stream.h
index fccf1f8dba1a6cf437be5ccbec861d708f98b308..cf06abb4ddefa8e5f8a68ba837e02b0bfc8f0dfb 100644 (file)
--- a/include/haproxy/quic_stream.h
+++ b/include/haproxy/quic_stream.h
@@ -12,7 +12,7 @@ struct qc_stream_desc *qc_stream_desc_new(uint64_t id, enum qcs_type, void *ctx,
                                           struct quic_conn *qc);
 void qc_stream_desc_release(struct qc_stream_desc *stream, uint64_t final_size,
                             void *new_ctx);
-int qc_stream_desc_ack(struct qc_stream_desc **stream, size_t offset, size_t len, int fin);
+int qc_stream_desc_ack(struct qc_stream_desc *stream, size_t offset, size_t len, int fin);
 void qc_stream_desc_free(struct qc_stream_desc *stream, int closing);
 
 struct buffer *qc_stream_buf_get(struct qc_stream_desc *stream);
@@ -21,6 +21,13 @@ struct buffer *qc_stream_buf_alloc(struct qc_stream_desc *stream,
 struct buffer *qc_stream_buf_realloc(struct qc_stream_desc *stream);
 void qc_stream_buf_release(struct qc_stream_desc *stream);
 
+/* Returns true if nothing to ack yet for stream <s> including FIN bit. */
+static inline int qc_stream_desc_done(const struct qc_stream_desc *s)
+{
+       return (s->flags & (QC_SD_FL_RELEASE|QC_SD_FL_WAIT_FOR_FIN)) == QC_SD_FL_RELEASE &&
+              LIST_ISEMPTY(&s->buf_list);
+}
+
 /* Reports emission of STREAM frame starting at <offset> and of length <len>,
  * related to <stream> data storage.
  */

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 82a4b5d895c7984f0af8504632c9ed6d056b7ca1..788f053f6c6f0883fc550488a30feea835bf9f24 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -219,7 +219,7 @@ static int quic_stream_try_to_consume(struct quic_conn *qc,
                struct qf_stream *strm_frm;
                struct quic_frame *frm;
                size_t offset, len;
-               int fin;
+               int fin, ack;
 
                strm_frm = eb64_entry(frm_node, struct qf_stream, offset);
                frm = container_of(strm_frm, struct quic_frame, stream);
@@ -230,21 +230,11 @@ static int quic_stream_try_to_consume(struct quic_conn *qc,
                if (offset > stream->ack_offset)
                        break;
 
-               if (qc_stream_desc_ack(&stream, offset, len, fin)) {
-                       /* cf. next comment : frame may be freed at this stage. */
+               ack = qc_stream_desc_ack(stream, offset, len, fin);
+               if (ack) {
                        TRACE_DEVEL("stream consumed", QUIC_EV_CONN_ACKSTRM,
-                                   qc, stream ? strm_frm : NULL, stream);
-                       ret = 1;
-               }
-
-               /* If stream is NULL after qc_stream_desc_ack(), it means frame
-                * has been freed. with the stream frames tree. Nothing to do
-                * anymore in here.
-                */
-               if (!stream) {
-                       qc_check_close_on_released_mux(qc);
+                                   qc, strm_frm, stream);
                        ret = 1;
-                       goto leave;
                }
 
                frm_node = eb64_next(frm_node);
@@ -253,7 +243,6 @@ static int quic_stream_try_to_consume(struct quic_conn *qc,
                qc_release_frm(qc, frm);
        }
 
- leave:
        TRACE_LEAVE(QUIC_EV_CONN_ACKSTRM, qc);
        return ret;
 }
@@ -276,6 +265,7 @@ static void qc_handle_newly_acked_frm(struct quic_conn *qc, struct quic_frame *f
                const size_t offset = strm_frm->offset.key;
                const size_t len = strm_frm->len;
                const int fin = frm->type & QUIC_STREAM_FRAME_TYPE_FIN_BIT;
+               int ack;
 
                /* do not use strm_frm->stream as the qc_stream_desc instance
                 * might be freed at this stage. Use the id to do a proper
@@ -295,28 +285,25 @@ static void qc_handle_newly_acked_frm(struct quic_conn *qc, struct quic_frame *f
 
                TRACE_DEVEL("acked stream", QUIC_EV_CONN_ACKSTRM, qc, strm_frm, stream);
                if (offset <= stream->ack_offset) {
-                       if (qc_stream_desc_ack(&stream, offset, len, fin)) {
+                       ack = qc_stream_desc_ack(stream, offset, len, fin);
+                       if (ack || fin) {
                                TRACE_DEVEL("stream consumed", QUIC_EV_CONN_ACKSTRM,
                                            qc, strm_frm, stream);
-                       }
 
-                       if (!stream) {
-                               /* no need to continue if stream freed. */
-                               TRACE_DEVEL("stream released and freed", QUIC_EV_CONN_ACKSTRM, qc);
-                               qc_release_frm(qc, frm);
-                               qc_check_close_on_released_mux(qc);
-                               break;
+                               quic_stream_try_to_consume(qc, stream);
+
+                               if (qc_stream_desc_done(stream)) {
+                                       /* no need to continue if stream freed. */
+                                       TRACE_DEVEL("stream released and freed", QUIC_EV_CONN_ACKSTRM, qc);
+                                       qc_check_close_on_released_mux(qc);
+                               }
                        }
 
-                       TRACE_DEVEL("stream consumed", QUIC_EV_CONN_ACKSTRM,
-                                   qc, strm_frm, stream);
                        qc_release_frm(qc, frm);
                }
                else {
                        eb64_insert(&stream->acked_frms, &strm_frm->offset);
                }
-
-               quic_stream_try_to_consume(qc, stream);
        }
        break;
        default:

diff --git a/src/quic_stream.c b/src/quic_stream.c
index 4aead31ff18f833f1685bf374bdc5e96d707b938..7d54fab438e070e85d7fb869a1a6d70361da7a91 100644 (file)
--- a/src/quic_stream.c
+++ b/src/quic_stream.c
@@ -19,12 +19,6 @@ DECLARE_STATIC_POOL(pool_head_quic_stream_buf, "qc_stream_buf",
 
 static struct pool_head *pool_head_sbuf;
 
-/* Returns true if nothing to ack yet for stream <s> including FIN bit. */
-static inline int qc_stream_desc_done(const struct qc_stream_desc *s)
-{
-       return !(s->flags & QC_SD_FL_WAIT_FOR_FIN) && LIST_ISEMPTY(&s->buf_list);
-}
-
 static void qc_stream_buf_free(struct qc_stream_desc *stream,
                                struct qc_stream_buf **stream_buf)
 {
@@ -139,58 +133,48 @@ void qc_stream_desc_release(struct qc_stream_desc *stream,
 }
 
 /* Acknowledge data at <offset> of length <len> for <stream> with <fin> set for
- * the final data. After data removal, if the stream does not contains data
- * any more and is already released, the instance stream is freed. <stream> is
- * set to NULL to indicate this.
+ * the final data.
  *
- * Returns the count of byte removed from stream. Do not forget to check if
- * <stream> is NULL after invocation.
+ * Returns the count of byte removed from stream.
  */
-int qc_stream_desc_ack(struct qc_stream_desc **stream, size_t offset, size_t len,
+int qc_stream_desc_ack(struct qc_stream_desc *stream, size_t offset, size_t len,
                        int fin)
 {
-       struct qc_stream_desc *s = *stream;
        struct qc_stream_buf *stream_buf = NULL;
        struct buffer *buf = NULL;
        size_t diff;
 
        /* Cannot advertise FIN for an inferior data range. */
-       BUG_ON(fin && offset + len < s->ack_offset);
+       BUG_ON(fin && offset + len < stream->ack_offset);
 
        /* No support now for out-of-order ACK reporting. */
-       BUG_ON(offset > s->ack_offset);
+       BUG_ON(offset > stream->ack_offset);
 
-       if (offset + len < s->ack_offset)
+       if (offset + len < stream->ack_offset)
                return 0;
 
-       diff = offset + len - s->ack_offset;
+       diff = offset + len - stream->ack_offset;
        if (diff) {
                /* Buf list cannot be empty if there is still unacked data. */
-               BUG_ON(LIST_ISEMPTY(&s->buf_list));
+               BUG_ON(LIST_ISEMPTY(&stream->buf_list));
 
                /* get oldest buffer from buf_list */
-               stream_buf = LIST_NEXT(&s->buf_list, struct qc_stream_buf *, list);
+               stream_buf = LIST_NEXT(&stream->buf_list, struct qc_stream_buf *, list);
                buf = &stream_buf->buf;
 
-               s->ack_offset += diff;
+               stream->ack_offset += diff;
                b_del(buf, diff);
 
                /* Free oldest buffer if all data acknowledged. */
                if (!b_data(buf)) {
-                       qc_stream_buf_free(s, &stream_buf);
+                       qc_stream_buf_free(stream, &stream_buf);
                        buf = NULL;
                }
        }
 
        if (fin) {
                /* Mark FIN as acknowledged. */
-               s->flags &= ~QC_SD_FL_WAIT_FOR_FIN;
-       }
-
-       /* Free stream instance if already released and everything acknowledged. */
-       if ((s->flags & QC_SD_FL_RELEASE) && qc_stream_desc_done(s)) {
-               qc_stream_desc_free(s, 0);
-               *stream = NULL;
+               stream->flags &= ~QC_SD_FL_WAIT_FOR_FIN;
        }
 
        return diff;