[MAJOR] buffers: fix the BF_EMPTY flag's meaning

The BF_EMPTY flag was once used to indicate an empty buffer. However,
it was used half the time as meaning the buffer is empty for the reader,
and half the time as meaning there is nothing left to send.

"nothing to send" is only indicated by "->send_max=0 && !pipe". Once
we fix this, we discover that the flag is not used anymore. So the
flags has been renamed BF_OUT_EMPTY and means exactly the condition
above, ie, there is nothing to send.

Doing so has allowed us to remove some unused tests for emptiness,
but also to uncover a certain amount of situations where the flag
was not correctly set or tested.

diff --git a/include/proto/buffers.h b/include/proto/buffers.h
index 8df6231a1e0b42fb7512389c123cf8acc1e6dd5d..6878cfe7314c0a3c17fc2da5172ca6d4b8001f46 100644 (file)
--- a/include/proto/buffers.h
+++ b/include/proto/buffers.h
@@ -42,7 +42,7 @@ int init_buffer();
  * so that the compiler can optimize it away if changed immediately after the
  * call to this function. By default, it is set to the full size of the buffer.
  * This implies that buffer_init() must only be called once ->size is set !
- * The BF_EMPTY flags is set.
+ * The BF_OUT_EMPTY flags is set.
  */
 static inline void buffer_init(struct buffer *buf)
 {
@@ -52,22 +52,11 @@ static inline void buffer_init(struct buffer *buf)
        buf->pipe = NULL;
        buf->analysers = 0;
        buf->cons = NULL;
-       buf->flags = BF_EMPTY;
+       buf->flags = BF_OUT_EMPTY;
        buf->r = buf->lr = buf->w = buf->data;
        buf->max_len = buf->size;
 }
 
-/* returns 1 if the buffer is empty, 0 otherwise */
-static inline int buffer_isempty(const struct buffer *buf)
-{
-       return buf->l == 0;
-}
-
-/* returns 1 if the buffer is full, 0 otherwise */
-static inline int buffer_isfull(const struct buffer *buf) {
-       return buf->l == buf->size;
-}
-
 /* Check buffer timeouts, and set the corresponding flags. The
  * likely/unlikely have been optimized for fastest normal path.
  * The read/write timeouts are not set if there was activity on the buffer.
@@ -100,6 +89,9 @@ static inline void buffer_forward(struct buffer *buf, unsigned int bytes)
 {
        unsigned int data_left;
 
+       if (!bytes)
+               return;
+       buf->flags &= ~BF_OUT_EMPTY;
        data_left = buf->l - buf->send_max;
        if (data_left >= bytes) {
                buf->send_max += bytes;
@@ -118,6 +110,8 @@ static inline void buffer_flush(struct buffer *buf)
 {
        if (buf->send_max < buf->l)
                buf->send_max = buf->l;
+       if (buf->send_max)
+               buf->flags &= ~BF_OUT_EMPTY;
 }
 
 /* Erase any content from buffer <buf> and adjusts flags accordingly. Note
@@ -130,9 +124,11 @@ static inline void buffer_erase(struct buffer *buf)
        buf->to_forward = 0;
        buf->r = buf->lr = buf->w = buf->data;
        buf->l = 0;
-       buf->flags |= BF_EMPTY | BF_FULL;
-       if (buf->max_len)
-               buf->flags &= ~BF_FULL;
+       buf->flags &= ~(BF_FULL | BF_OUT_EMPTY);
+       if (!buf->pipe)
+               buf->flags |= BF_OUT_EMPTY;
+       if (!buf->max_len)
+               buf->flags |= BF_FULL;
 }
 
 /* Cut the "tail" of the buffer, which means strip it to the length of unsent
@@ -154,7 +150,7 @@ static inline void buffer_cut_tail(struct buffer *buf)
        if (buf->r >= buf->data + buf->size)
                buf->r -= buf->size;
        buf->lr = buf->r;
-       buf->flags &= ~(BF_EMPTY|BF_FULL);
+       buf->flags &= ~BF_FULL;
        if (buf->l >= buf->max_len)
                buf->flags |= BF_FULL;
 }
@@ -336,16 +332,15 @@ static inline void buffer_skip(struct buffer *buf, int len)
                buf->w = buf->data; /* wrap around the buffer */
 
        buf->l -= len;
-       if (!buf->l) {
+       if (!buf->l)
                buf->r = buf->w = buf->lr = buf->data;
-               if (!buf->pipe)
-                       buf->flags |= BF_EMPTY;
-       }
 
        if (buf->l < buf->max_len)
                buf->flags &= ~BF_FULL;
 
        buf->send_max -= len;
+       if (!buf->send_max && !buf->pipe)
+               buf->flags |= BF_OUT_EMPTY;
 }
 
 /*
@@ -375,11 +370,6 @@ static inline int buffer_si_putchar(struct buffer *buf, char c)
        if (buf->flags & BF_FULL)
                return 0;
 
-       if (buf->flags & BF_EMPTY) {
-               buf->flags &= ~BF_EMPTY;
-               buf->r = buf->w = buf->lr = buf->data;
-       }
-
        *buf->r = c;
 
        buf->l++;
@@ -393,6 +383,7 @@ static inline int buffer_si_putchar(struct buffer *buf, char c)
        if ((signed)(buf->to_forward - 1) >= 0) {
                buf->to_forward--;
                buf->send_max++;
+               buf->flags &= ~BF_OUT_EMPTY;
        }
 
        buf->total++;

diff --git a/include/types/buffers.h b/include/types/buffers.h
index b758596069caf7a65c3f9dd0046f41d8b7337158..969c10342715565b9dd2f4742c1c0a7a50988658 100644 (file)
--- a/include/types/buffers.h
+++ b/include/types/buffers.h
@@ -68,7 +68,7 @@
 #define BF_WRITE_ERROR    0x000800  /* unrecoverable error on consumer side */
 #define BF_WRITE_ACTIVITY (BF_WRITE_NULL|BF_WRITE_PARTIAL|BF_WRITE_ERROR)
 
-#define BF_EMPTY          0x001000  /* buffer is empty */
+#define BF_OUT_EMPTY      0x001000  /* send_max and pipe are empty. Set by last change. */
 #define BF_SHUTW          0x002000  /* consumer has already shut down */
 #define BF_SHUTW_NOW      0x004000  /* the consumer must shut down for writes ASAP */
 #define BF_AUTO_CLOSE     0x008000  /* producer can forward shutdown to other side */
@@ -120,7 +120,7 @@
 #define BF_MASK_ANALYSER        (BF_READ_ATTACHED|BF_READ_ACTIVITY|BF_READ_TIMEOUT|BF_ANA_TIMEOUT|BF_WRITE_ACTIVITY)
 
 /* Mask for static flags which are not events, but might change during processing */
-#define BF_MASK_STATIC          (BF_EMPTY|BF_FULL|BF_HIJACK|BF_AUTO_CLOSE|BF_AUTO_CONNECT|BF_SHUTR|BF_SHUTW|BF_SHUTR_NOW|BF_SHUTW_NOW)
+#define BF_MASK_STATIC          (BF_OUT_EMPTY|BF_FULL|BF_HIJACK|BF_AUTO_CLOSE|BF_AUTO_CONNECT|BF_SHUTR|BF_SHUTW|BF_SHUTR_NOW|BF_SHUTW_NOW)
 
 
 /* Analysers (buffer->analysers).

diff --git a/src/buffers.c b/src/buffers.c
index edda45954fa0227e2001fac357b8359632d01267..6b551ab2c2683d5fb2e3766280f2f55cfbdb0643 100644 (file)
--- a/src/buffers.c
+++ b/src/buffers.c
@@ -64,9 +64,7 @@ int buffer_write(struct buffer *buf, const char *msg, int len)
        if (buf->r == buf->data + buf->size)
                buf->r = buf->data;
 
-       buf->flags &= ~(BF_EMPTY|BF_FULL);
-       if (buf->l == 0)
-               buf->flags |= BF_EMPTY;
+       buf->flags &= ~(BF_OUT_EMPTY|BF_FULL);
        if (buf->l >= buf->max_len)
                buf->flags |= BF_FULL;
 
@@ -107,12 +105,13 @@ int buffer_feed(struct buffer *buf, const char *str, int len)
                int fwd = MIN(buf->to_forward, len);
                buf->send_max   += fwd;
                buf->to_forward -= fwd;
+               buf->flags &= ~BF_OUT_EMPTY;
        }
 
        if (buf->r == buf->data + buf->size)
                buf->r = buf->data;
 
-       buf->flags &= ~(BF_EMPTY|BF_FULL);
+       buf->flags &= ~BF_FULL;
        if (buf->l >= buf->max_len)
                buf->flags |= BF_FULL;
 
@@ -174,6 +173,8 @@ int buffer_si_peekline(struct buffer *buf, char *str, int len)
  * <b>'s parameters (l, r, w, h, lr) are recomputed to be valid after the shift.
  * the shift value (positive or negative) is returned.
  * If there's no space left, the move is not done.
+ * The function does not adjust ->send_max nor BF_OUT_EMPTY because it does not
+ * make sense to use it on data scheduled to be sent.
  *
  */
 int buffer_replace(struct buffer *b, char *pos, char *end, const char *str)
@@ -199,9 +200,9 @@ int buffer_replace(struct buffer *b, char *pos, char *end, const char *str)
        if (b->lr > pos) b->lr += delta;
        b->l += delta;
 
-       b->flags &= ~(BF_EMPTY|BF_FULL);
+       b->flags &= ~BF_FULL;
        if (b->l == 0)
-               b->flags |= BF_EMPTY;
+               b->r = b->w = b->lr = b->data;
        if (b->l >= b->max_len)
                b->flags |= BF_FULL;
 
@@ -240,9 +241,9 @@ int buffer_replace2(struct buffer *b, char *pos, char *end, const char *str, int
        if (b->lr > pos) b->lr += delta;
        b->l += delta;
 
-       b->flags &= ~(BF_EMPTY|BF_FULL);
+       b->flags &= ~BF_FULL;
        if (b->l == 0)
-               b->flags |= BF_EMPTY;
+               b->r = b->w = b->lr = b->data;
        if (b->l >= b->max_len)
                b->flags |= BF_FULL;
 
@@ -284,9 +285,7 @@ int buffer_insert_line2(struct buffer *b, char *pos, const char *str, int len)
        if (b->lr > pos) b->lr += delta;
        b->l += delta;
 
-       b->flags &= ~(BF_EMPTY|BF_FULL);
-       if (b->l == 0)
-               b->flags |= BF_EMPTY;
+       b->flags &= ~BF_FULL;
        if (b->l >= b->max_len)
                b->flags |= BF_FULL;
 

diff --git a/src/session.c b/src/session.c
index 6c9390d118decd25e33352648210132fac706baf..01ce306438236aa99f1a5f19a800f731b41b3d78 100644 (file)
--- a/src/session.c
+++ b/src/session.c
@@ -200,7 +200,7 @@ int sess_update_st_con_tcp(struct session *s, struct stream_interface *si)
        /* OK, maybe we want to abort */
        if (unlikely((rep->flags & BF_SHUTW) ||
                     ((req->flags & BF_SHUTW_NOW) && /* FIXME: this should not prevent a connection from establishing */
-                     (((req->flags & (BF_EMPTY|BF_WRITE_ACTIVITY)) == BF_EMPTY) ||
+                     (((req->flags & (BF_OUT_EMPTY|BF_WRITE_ACTIVITY)) == BF_OUT_EMPTY) ||
                       s->be->options & PR_O_ABRT_CLOSE)))) {
                /* give up */
                si->shutw(si);
@@ -452,7 +452,7 @@ void sess_update_stream_int(struct session *s, struct stream_interface *si)
                /* Connection remains in queue, check if we have to abort it */
                if ((si->ob->flags & (BF_READ_ERROR)) ||
                    ((si->ob->flags & BF_SHUTW_NOW) &&   /* empty and client aborted */
-                    (si->ob->flags & BF_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
+                    (si->ob->flags & BF_OUT_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
                        /* give up */
                        si->exp = TICK_ETERNITY;
                        s->logs.t_queue = tv_ms_elapsed(&s->logs.tv_accept, &now);
@@ -472,7 +472,7 @@ void sess_update_stream_int(struct session *s, struct stream_interface *si)
                /* Connection request might be aborted */
                if ((si->ob->flags & (BF_READ_ERROR)) ||
                    ((si->ob->flags & BF_SHUTW_NOW) &&  /* empty and client aborted */
-                    (si->ob->flags & BF_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
+                    (si->ob->flags & BF_OUT_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
                        /* give up */
                        si->exp = TICK_ETERNITY;
                        si->shutr(si);
@@ -992,11 +992,11 @@ resync_stream_interface:
                             ((s->req->cons->state == SI_ST_EST) &&
                              (s->be->options & PR_O_FORCE_CLO) &&
                              (s->rep->flags & BF_READ_ACTIVITY))))
-               buffer_shutw_now(s->req);
+                       buffer_shutw_now(s->req);
        }
 
        /* shutdown(write) pending */
-       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_EMPTY)) == (BF_SHUTW_NOW|BF_EMPTY)))
+       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_OUT_EMPTY)) == (BF_SHUTW_NOW|BF_OUT_EMPTY)))
                s->req->cons->shutw(s->req->cons);
 
        /* shutdown(write) done on server side, we must stop the client too */
@@ -1015,8 +1015,7 @@ resync_stream_interface:
         */
        if (s->req->cons->state == SI_ST_INI) {
                if (!(s->req->flags & (BF_SHUTW|BF_SHUTW_NOW))) {
-                       if ((s->req->flags & BF_AUTO_CONNECT) ||
-                           (s->req->send_max || s->req->pipe)) {
+                       if ((s->req->flags & (BF_AUTO_CONNECT|BF_OUT_EMPTY)) != BF_OUT_EMPTY) {
                                /* If we have a ->connect method, we need to perform a connection request,
                                 * otherwise we immediately switch to the connected state.
                                 */
@@ -1112,7 +1111,7 @@ resync_stream_interface:
                buffer_shutw_now(s->rep);
 
        /* shutdown(write) pending */
-       if (unlikely((s->rep->flags & (BF_SHUTW|BF_EMPTY|BF_SHUTW_NOW)) == (BF_EMPTY|BF_SHUTW_NOW)))
+       if (unlikely((s->rep->flags & (BF_SHUTW|BF_OUT_EMPTY|BF_SHUTW_NOW)) == (BF_OUT_EMPTY|BF_SHUTW_NOW)))
                s->rep->cons->shutw(s->rep->cons);
 
        /* shutdown(write) done on the client side, we must stop the server too */

diff --git a/src/stream_sock.c b/src/stream_sock.c
index 0ec83cd7f29787df1595309fea882c0edc739d98..cf6da5a97aa9849e49bb10fb9ddb2ff700ad735d 100644 (file)
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -94,7 +94,7 @@ _syscall6(int, splice, int, fdin, loff_t *, off_in, int, fdout, loff_t *, off_ou
  *   BF_READ_NULL
  *   BF_READ_PARTIAL
  *   BF_WRITE_PARTIAL (during copy)
- *   BF_EMPTY (during copy)
+ *   BF_OUT_EMPTY (during copy)
  *   SI_FL_ERR
  *   SI_FL_WAIT_ROOM
  *   (SI_FL_WAIT_RECV)
@@ -207,7 +207,7 @@ static int stream_sock_splice_in(struct buffer *b, struct stream_interface *si)
                b->total += ret;
                b->pipe->data += ret;
                b->flags |= BF_READ_PARTIAL;
-               b->flags &= ~BF_EMPTY; /* to prevent shutdowns */
+               b->flags &= ~BF_OUT_EMPTY;
 
                if (b->pipe->data >= SPLICE_FULL_HINT ||
                    ret >= global.tune.recv_enough) {
@@ -336,13 +336,13 @@ int stream_sock_read(int fd) {
                                int fwd = MIN(b->to_forward, ret);
                                b->send_max   += fwd;
                                b->to_forward -= fwd;
+                               b->flags &= ~BF_OUT_EMPTY;
                        }
 
                        if (fdtab[fd].state == FD_STCONN)
                                fdtab[fd].state = FD_STREADY;
 
                        b->flags |= BF_READ_PARTIAL;
-                       b->flags &= ~BF_EMPTY;
 
                        if (b->r == b->data + b->size) {
                                b->r = b->data; /* wrap around the buffer */
@@ -466,7 +466,7 @@ int stream_sock_read(int fd) {
 
  out_wakeup:
        /* We might have some data the consumer is waiting for */
-       if ((b->send_max || b->pipe) && (b->cons->flags & SI_FL_WAIT_DATA)) {
+       if (!(b->flags & BF_OUT_EMPTY) && (b->cons->flags & SI_FL_WAIT_DATA)) {
                int last_len = b->pipe ? b->pipe->data : 0;
 
                b->cons->chk_snd(b->cons);
@@ -566,13 +566,11 @@ static int stream_sock_write_loop(struct stream_interface *si, struct buffer *b)
        /* At this point, the pipe is empty, but we may still have data pending
         * in the normal buffer.
         */
-       if (!b->l) {
-               b->flags |= BF_EMPTY;
-               return retval;
-       }
 #endif
-       if (!b->send_max)
+       if (!b->send_max) {
+               b->flags |= BF_OUT_EMPTY;
                return retval;
+       }
 
        /* when we're in this loop, we already know that there is no spliced
         * data left, and that there are sendable buffered data.
@@ -634,16 +632,16 @@ static int stream_sock_write_loop(struct stream_interface *si, struct buffer *b)
                        if (likely(b->l < b->max_len))
                                b->flags &= ~BF_FULL;
 
-                       if (likely(!b->l)) {
+                       if (likely(!b->l))
                                /* optimize data alignment in the buffer */
                                b->r = b->w = b->lr = b->data;
-                               if (likely(!b->pipe))
-                                       b->flags |= BF_EMPTY;
-                       }
 
                        b->send_max -= ret;
-                       if (!b->send_max || !b->l)
+                       if (!b->send_max) {
+                               if (likely(!b->pipe))
+                                       b->flags |= BF_OUT_EMPTY;
                                break;
+                       }
 
                        /* if the system buffer is full, don't insist */
                        if (ret < max)
@@ -691,7 +689,7 @@ int stream_sock_write(int fd)
        if (b->flags & BF_SHUTW)
                goto out_wakeup;
 
-       if (likely(!(b->flags & BF_EMPTY))) {
+       if (likely(!(b->flags & BF_OUT_EMPTY))) {
                /* OK there are data waiting to be sent */
                retval = stream_sock_write_loop(si, b);
                if (retval < 0)
@@ -735,7 +733,7 @@ int stream_sock_write(int fd)
                 */
        }
 
-       if (!b->pipe && !b->send_max) {
+       if (b->flags & BF_OUT_EMPTY) {
                /* the connection is established but we can't write. Either the
                 * buffer is empty, or we just refrain from sending because the
                 * send_max limit was reached. Maybe we just wrote the last
@@ -747,7 +745,7 @@ int stream_sock_write(int fd)
                        goto out_wakeup;
                }
                
-               if ((b->flags & (BF_EMPTY|BF_SHUTW)) == BF_EMPTY)
+               if ((b->flags & (BF_SHUTW|BF_FULL|BF_HIJACK)) == 0)
                        si->flags |= SI_FL_WAIT_DATA;
 
                EV_FD_CLR(fd, DIR_WR);
@@ -757,8 +755,7 @@ int stream_sock_write(int fd)
  out_may_wakeup:
        if (b->flags & BF_WRITE_ACTIVITY) {
                /* update timeout if we have written something */
-               if ((b->send_max || b->pipe) &&
-                   (b->flags & (BF_SHUTW|BF_WRITE_PARTIAL)) == BF_WRITE_PARTIAL)
+               if ((b->flags & (BF_OUT_EMPTY|BF_SHUTW|BF_WRITE_PARTIAL)) == BF_WRITE_PARTIAL)
                        b->wex = tick_add_ifset(now_ms, b->wto);
 
        out_wakeup:
@@ -781,7 +778,7 @@ int stream_sock_write(int fd)
                 * any more data to forward and it's not planned to send any more.
                 */
                if (likely((b->flags & (BF_WRITE_NULL|BF_WRITE_ERROR|BF_SHUTW)) ||
-                          (!b->to_forward && !b->send_max && !b->pipe) ||
+                          ((b->flags & BF_OUT_EMPTY) && !b->to_forward) ||
                           si->state != SI_ST_EST ||
                           b->prod->state != SI_ST_EST))
                        task_wakeup(si->owner, TASK_WOKEN_IO);
@@ -925,9 +922,9 @@ void stream_sock_data_finish(struct stream_interface *si)
        /* Check if we need to close the write side */
        if (!(ob->flags & BF_SHUTW)) {
                /* Write not closed, update FD status and timeout for writes */
-               if ((ob->send_max == 0 && !ob->pipe) || (ob->flags & BF_EMPTY)) {
+               if (ob->flags & BF_OUT_EMPTY) {
                        /* stop writing */
-                       if ((ob->flags & (BF_EMPTY|BF_HIJACK)) == BF_EMPTY)
+                       if ((ob->flags & (BF_FULL|BF_HIJACK)) == 0)
                                si->flags |= SI_FL_WAIT_DATA;
                        EV_FD_COND_C(fd, DIR_WR);
                        ob->wex = TICK_ETERNITY;
@@ -1011,7 +1008,7 @@ void stream_sock_chk_snd(struct stream_interface *si)
 
        if (!(si->flags & SI_FL_WAIT_DATA) ||        /* not waiting for data */
            (fdtab[si->fd].ev & FD_POLL_OUT) ||      /* we'll be called anyway */
-           !(ob->send_max || ob->pipe))             /* called with nothing to send ! */
+           (ob->flags & BF_OUT_EMPTY))              /* called with nothing to send ! */
                return;
 
        retval = stream_sock_write_loop(si, ob);
@@ -1035,7 +1032,7 @@ void stream_sock_chk_snd(struct stream_interface *si)
         * been sent, and that we may have to poll first. We have to do that
         * too if the buffer is not empty.
         */
-       if (ob->send_max == 0 && !ob->pipe) {
+       if (ob->flags & BF_OUT_EMPTY) {
                /* the connection is established but we can't write. Either the
                 * buffer is empty, or we just refrain from sending because the
                 * send_max limit was reached. Maybe we just wrote the last
@@ -1048,7 +1045,7 @@ void stream_sock_chk_snd(struct stream_interface *si)
                        goto out_wakeup;
                }
 
-               if ((ob->flags & (BF_SHUTW|BF_EMPTY|BF_HIJACK)) == BF_EMPTY)
+               if ((ob->flags & (BF_SHUTW|BF_FULL|BF_HIJACK)) == 0)
                        si->flags |= SI_FL_WAIT_DATA;
                ob->wex = TICK_ETERNITY;
        }
@@ -1064,8 +1061,7 @@ void stream_sock_chk_snd(struct stream_interface *si)
 
        if (likely(ob->flags & BF_WRITE_ACTIVITY)) {
                /* update timeout if we have written something */
-               if ((ob->send_max || ob->pipe) &&
-                   (ob->flags & (BF_SHUTW|BF_WRITE_PARTIAL)) == BF_WRITE_PARTIAL)
+               if ((ob->flags & (BF_OUT_EMPTY|BF_SHUTW|BF_WRITE_PARTIAL)) == BF_WRITE_PARTIAL)
                        ob->wex = tick_add_ifset(now_ms, ob->wto);
 
                if (tick_isset(si->ib->rex)) {
@@ -1083,7 +1079,7 @@ void stream_sock_chk_snd(struct stream_interface *si)
         * have to notify the task.
         */
        if (likely((ob->flags & (BF_WRITE_NULL|BF_WRITE_ERROR|BF_SHUTW)) ||
-                  (!ob->to_forward && !ob->send_max && !ob->pipe) ||
+                  ((ob->flags & BF_OUT_EMPTY) && !ob->to_forward) ||
                   si->state != SI_ST_EST)) {
        out_wakeup:
                task_wakeup(si->owner, TASK_WOKEN_IO);