MAJOR: start to change buffer API
This is intentionally the minimal and safest set of changes, some cleanups
area still required. These changes are quite tricky and cannot be
independantly tested, so it's important to keep this patch as bisectable
as possible.
buf_empty and buf_wanted were changed and are now exactly similar since
there's no <p> member in the structure anymore. Given that no test is
ever made in the code to check that buf == &buf_wanted, it may be possible
that we don't need to have two anymore, unless some buf_empty tests have
precedence. This will have to be investigated.
A significant part of this commit affects the HTTP compression code,
which used to deeply manipulate the input and output buffers without
any reasonable solution for a better abstraction. For this reason, if
any regression is met and designates this patch as the culprit, it is
important to run tests which specifically involve compression or which
definitely don't use it in order to spot the issue.
Cc: Olivier Houchard <ohouchard@haproxy.com>
diff --git a/include/common/buf.h b/include/common/buf.h
index d9de073..02a66b3 100644
--- a/include/common/buf.h
+++ b/include/common/buf.h
@@ -32,10 +32,10 @@
/* Structure defining a buffer's head */
struct buffer {
- char *p; /* buffer's start pointer, separates in and out data */
+ size_t head; /* start offset of remaining data relative to data */
+ size_t len; /* length of data after head */
size_t size; /* buffer size in bytes */
- size_t i; /* number of input bytes pending for analysis in the buffer */
- size_t o; /* number of out bytes the sender can consume from this buffer */
+ size_t output; /* TEMPORARY: part of <len> which is to be forwarded */
char data[0]; /* <size> bytes of stored data */
};
@@ -73,7 +73,7 @@
/* b_data() : returns the number of bytes present in the buffer. */
static inline size_t b_data(const struct buffer *b)
{
- return b->i + b->o;
+ return b->len;
}
/* b_room() : returns the amount of room left in the buffer */
@@ -95,12 +95,12 @@
*/
static inline size_t __b_stop_ofs(const struct buffer *b)
{
- return b->p - b->data + b->i;
+ return b->head + b->len;
}
static inline const char *__b_stop(const struct buffer *b)
{
- return b->p + b->i;
+ return b_orig(b) + __b_stop_ofs(b);
}
static inline size_t b_stop_ofs(const struct buffer *b)
@@ -114,7 +114,7 @@
static inline const char *b_stop(const struct buffer *b)
{
- return b->data + b_stop_ofs(b);
+ return b_orig(b) + b_stop_ofs(b);
}
@@ -125,32 +125,27 @@
*/
static inline size_t __b_peek_ofs(const struct buffer *b, size_t ofs)
{
- return b->p - b->data + ofs - b->o;
+ return b->head + ofs;
}
static inline char *__b_peek(const struct buffer *b, size_t ofs)
{
- return b->p - b->o + ofs;
+ return b_orig(b) + __b_peek_ofs(b, ofs);
}
static inline size_t b_peek_ofs(const struct buffer *b, size_t ofs)
{
size_t ret = __b_peek_ofs(b, ofs);
- if (ret >= b->size) {
- /* wraps either up or down */
- if ((ssize_t)ret < 0)
- ret += b->size;
- else
- ret -= b->size;
- }
+ if (ret >= b->size)
+ ret -= b->size;
return ret;
}
static inline char *b_peek(const struct buffer *b, size_t ofs)
{
- return (char *)b->data + b_peek_ofs(b, ofs);
+ return b_orig(b) + b_peek_ofs(b, ofs);
}
@@ -160,22 +155,22 @@
*/
static inline size_t __b_head_ofs(const struct buffer *b)
{
- return __b_peek_ofs(b, 0);
+ return b->head;
}
static inline char *__b_head(const struct buffer *b)
{
- return __b_peek(b, 0);
+ return b_orig(b) + __b_head_ofs(b);
}
static inline size_t b_head_ofs(const struct buffer *b)
{
- return b_peek_ofs(b, 0);
+ return __b_head_ofs(b);
}
static inline char *b_head(const struct buffer *b)
{
- return b_peek(b, 0);
+ return __b_head(b);
}
@@ -248,20 +243,11 @@
}
/* b_space_wraps() : returns non-zero only if the buffer's free space wraps :
- * [ |oooo| ] => yes
- * [ |iiii| ] => yes
- * [ |oooo|iiii| ] => yes
- * [oooo| ] => no
- * [ |oooo] => no
- * [iiii| ] => no
- * [ |iiii] => no
- * [oooo|iiii| ] => no
- * [ |oooo|iiii] => no
- * [iiii| |oooo] => no
- * [oo|iiii| |oo] => no
- * [iiii| |oo|ii] => no
- * [oooooooooo|iiiiiiiiiii] => no
- * [iiiiiiiiiiiii|oooooooo] => no
+ * [ |xxxx| ] => yes
+ * [xxxx| ] => no
+ * [ |xxxx] => no
+ * [xxxx| |xxxx] => no
+ * [xxxxxxxxxx|xxxxxxxxxxx] => no
*
* So the only case where the buffer does not wrap is when there's data either
* at the beginning or at the end of the buffer. Thus we have this :
@@ -379,32 +365,29 @@
/* b_reset() : resets a buffer. The size is not touched. */
static inline void b_reset(struct buffer *b)
{
- b->o = 0;
- b->i = 0;
- b->p = b_orig(b);
+ b->head = 0;
+ b->len = 0;
+ b->output = 0;
}
/* b_sub() : decreases the buffer length by <count> */
static inline void b_sub(struct buffer *b, size_t count)
{
- b->i -= count;
+ b->len -= count;
}
/* b_add() : increase the buffer length by <count> */
static inline void b_add(struct buffer *b, size_t count)
{
- b->i += count;
+ b->len += count;
}
/* b_set_data() : sets the buffer's length */
static inline void b_set_data(struct buffer *b, size_t len)
{
- if (len >= b->o)
- b->i = len - b->o;
- else {
- b->o = len;
- b->i = 0;
- }
+ if (len < b->output)
+ b->output = len;
+ b->len = len;
}
/* b_del() : skips <del> bytes in a buffer <b>. Covers both the output and the
@@ -413,22 +396,21 @@
*/
static inline void b_del(struct buffer *b, size_t del)
{
- if (del <= b->o) {
- b->o -= del;
- del = 0;
- }
- if (del) {
- b->p = b_peek(b, del);
- b->i -= del;
- del = 0;
- }
+ if (del >= b->output)
+ b->output = 0;
+ else
+ b->output -= del;
+ b->len -= del;
+ b->head += del;
+ if (b->head >= b->size)
+ b->head -= b->size;
}
/* b_realign_if_empty() : realigns a buffer if it's empty */
static inline void b_realign_if_empty(struct buffer *b)
{
if (!b_data(b))
- b->p = b->data;
+ b->head = 0;
}
/* b_slow_realign() : this function realigns a possibly wrapping buffer so that
@@ -470,7 +452,7 @@
memcpy(b_orig(b), swap, b_data(b) - output);
memcpy(b_wrap(b) - output, swap + b_size(b) - output, output);
- b->p = b->data;
+ b->head = b_size(b) - output;
}
#endif /* _COMMON_BUF_H */
diff --git a/include/common/buffer.h b/include/common/buffer.h
index 86eec16..fde7b03 100644
--- a/include/common/buffer.h
+++ b/include/common/buffer.h
@@ -104,16 +104,14 @@
return b_almost_full(buf);
}
-/* Cut the first <n> pending bytes in a contiguous buffer. It is illegal to
- * call this function with remaining data waiting to be sent (o > 0). The
- * caller must ensure that <n> is smaller than the actual buffer's length.
- * This is mainly used to remove empty lines at the beginning of a request
- * or a response.
+/* Cut the first <n> pending bytes in a contiguous buffer. The caller must
+ * ensure that <n> is smaller than the actual buffer's length. This is mainly
+ * used to remove empty lines at the beginning of a request or a response.
*/
static inline void bi_fast_delete(struct buffer *buf, int n)
{
- buf->i -= n;
- buf->p += n;
+ buf->len -= n;
+ buf->head += n;
}
/* This function writes the string <str> at position <pos> which must be in
@@ -128,16 +126,16 @@
return buffer_replace2(b, pos, end, str, strlen(str));
}
-/* Tries to write char <c> into output data at buffer <b>. Supports wrapping.
- * Data are truncated if buffer is full.
+/* Tries to append char <c> at the end of buffer <b>. Supports wrapping. Data
+ * are truncated if buffer is full.
*/
static inline void bo_putchr(struct buffer *b, char c)
{
if (b_data(b) == b->size)
return;
*b_tail(b) = c;
- b->p = b_peek(b, b->o + 1);
- b->o++;
+ b->len++;
+ b->output++;
}
/* Tries to append block <blk> at the end of buffer <b>. Supports wrapping.
@@ -158,13 +156,12 @@
half = len;
memcpy(b_tail(b), blk, half);
- b->p = b_peek(b, b->o + half);
- b->o += half;
+ b->len += half;
if (len > half) {
memcpy(b_tail(b), blk + half, len - half);
- b->p = b_peek(b, b->o + len - half);
- b->o += len - half;
+ b->len += len - half;
}
+ b->output += len;
return len;
}
@@ -194,7 +191,7 @@
if (b_data(b) == b->size)
return;
*b_tail(b) = c;
- b->i++;
+ b->len++;
}
/* Tries to append block <blk> at the end of buffer <b>. Supports wrapping.
@@ -215,10 +212,10 @@
half = len;
memcpy(b_tail(b), blk, half);
- b->i += half;
+ b->len += half;
if (len > half) {
memcpy(b_tail(b), blk + half, len - half);
- b->i += len - half;
+ b->len += len - half;
}
return len;
}
@@ -443,7 +440,7 @@
return r.len < b->size ? 0 : -1;
p = b_tail(b);
- b->i += r.len;
+ b->len += r.len;
while (r.len--) {
*p++ = *r.ptr++;
if (unlikely(p == end))
@@ -472,8 +469,8 @@
return r.len < b->size ? 0 : -1;
p = b_tail(b);
- b->p = b_peek(b, b->o + r.len);
- b->o += r.len;
+ b->len += r.len;
+ b->output += r.len;
while (r.len--) {
*p++ = *r.ptr++;
if (unlikely(p == end))
diff --git a/include/proto/channel.h b/include/proto/channel.h
index 8ca6695..291bb08 100644
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -128,7 +128,7 @@
/* co_data() : returns the amount of output data in the channel's buffer */
static inline size_t co_data(const struct channel *c)
{
- return c->buf->o;
+ return c->buf->output;
}
/* ci_data() : returns the amount of input data in the channel's buffer */
@@ -170,11 +170,7 @@
*/
static inline void c_adv(struct channel *c, size_t adv)
{
- struct buffer *b = c->buf;
-
- b->p = c_ptr(c, adv);
- b->i -= adv;
- b->o += adv;
+ c->buf->output += adv;
}
/* c_rew() : rewinds the channel's buffer by <adv> bytes, which means that the
@@ -184,11 +180,7 @@
*/
static inline void c_rew(struct channel *c, size_t adv)
{
- struct buffer *b = c->buf;
-
- b->p = c_ptr(c, (int)-adv);
- b->i += adv;
- b->o -= adv;
+ c->buf->output -= adv;
}
/* c_realign_if_empty() : realign the channel's buffer if it's empty */
@@ -200,7 +192,8 @@
/* Sets the amount of output for the channel */
static inline void co_set_data(struct channel *c, size_t output)
{
- c->buf->o = output;
+ c->buf->len += output - c->buf->output;
+ c->buf->output = output;
}
@@ -750,7 +743,7 @@
if (!ci_data(chn))
return;
- chn->buf->i = 0;
+ chn->buf->len = co_data(chn);
}
/* This function realigns a possibly wrapping channel buffer so that the input
diff --git a/src/buffer.c b/src/buffer.c
index a1ffec7..306f011 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -28,8 +28,8 @@
* what channel wants a buffer. They can reliably be exchanged, the split
* between the two is only an optimization.
*/
-struct buffer buf_empty = { .p = buf_empty.data };
-struct buffer buf_wanted = { .p = buf_wanted.data };
+struct buffer buf_empty = { };
+struct buffer buf_wanted = { };
/* list of objects waiting for at least one buffer */
struct list buffer_wq = LIST_HEAD_INIT(buffer_wq);
diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c
index 4972088..f8c82c4 100644
--- a/src/flt_http_comp.c
+++ b/src/flt_http_comp.c
@@ -38,6 +38,7 @@
static THREAD_LOCAL struct buffer *tmpbuf = &buf_empty;
static THREAD_LOCAL struct buffer *zbuf = &buf_empty;
+static THREAD_LOCAL unsigned int buf_output;
struct comp_state {
struct comp_ctx *comp_ctx; /* compression context */
@@ -56,13 +57,14 @@
struct stream *s,
struct http_msg *msg);
-static int http_compression_buffer_init(struct channel *inc, struct buffer *out);
+static int http_compression_buffer_init(struct channel *inc, struct buffer *out, unsigned int *out_len);
static int http_compression_buffer_add_data(struct comp_state *st,
struct buffer *in,
+ int in_out,
struct buffer *out, int sz);
static int http_compression_buffer_end(struct comp_state *st, struct stream *s,
struct channel *chn, struct buffer **out,
- int end);
+ unsigned int *out_len, int end);
/***********************************************************************/
static int
@@ -191,7 +193,7 @@
b_reset(tmpbuf);
c_adv(chn, fwd);
- ret = http_compression_buffer_init(chn, zbuf);
+ ret = http_compression_buffer_init(chn, zbuf, &buf_output);
c_rew(chn, fwd);
if (ret < 0) {
msg->chn->flags |= CF_WAKE_WRITE;
@@ -216,7 +218,7 @@
}
else {
c_adv(chn, *nxt);
- ret = http_compression_buffer_add_data(st, chn->buf, zbuf, len);
+ ret = http_compression_buffer_add_data(st, chn->buf, co_data(chn), zbuf, len);
c_rew(chn, *nxt);
if (ret < 0)
return ret;
@@ -242,7 +244,7 @@
b_reset(tmpbuf);
c_adv(chn, fwd);
- http_compression_buffer_init(chn, zbuf);
+ http_compression_buffer_init(chn, zbuf, &buf_output);
c_rew(chn, fwd);
st->initialized = 1;
}
@@ -296,17 +298,18 @@
}
if (msg->flags & HTTP_MSGF_TE_CHNK) {
- ret = http_compression_buffer_add_data(st, tmpbuf, zbuf, tmpbuf->i);
- if (ret != tmpbuf->i) {
+ ret = http_compression_buffer_add_data(st, tmpbuf, 0,
+ zbuf, b_data(tmpbuf));
+ if (ret != b_data(tmpbuf)) {
ha_warning("HTTP compression failed: Must consume %u bytes but only %d bytes consumed\n",
- (unsigned int)tmpbuf->i, ret);
+ (unsigned int)b_data(tmpbuf), ret);
return -1;
}
}
st->consumed = len - st->hdrs_len - st->tlrs_len;
c_adv(msg->chn, flt_rsp_fwd(filter) + st->hdrs_len);
- ret = http_compression_buffer_end(st, s, msg->chn, &zbuf, msg->msg_state >= HTTP_MSG_TRAILERS);
+ ret = http_compression_buffer_end(st, s, msg->chn, &zbuf, &buf_output, msg->msg_state >= HTTP_MSG_TRAILERS);
c_rew(msg->chn, flt_rsp_fwd(filter) + st->hdrs_len);
if (ret < 0)
return ret;
@@ -601,7 +604,7 @@
* Init HTTP compression
*/
static int
-http_compression_buffer_init(struct channel *inc, struct buffer *out)
+http_compression_buffer_init(struct channel *inc, struct buffer *out, unsigned int *out_len)
{
/* output stream requires at least 10 bytes for the gzip header, plus
* at least 8 bytes for the gzip trailer (crc+len), plus a possible
@@ -616,9 +619,8 @@
* cancel the operation later, it's cheap.
*/
b_reset(out);
- out->o = co_data(inc);
- out->p += out->o;
- out->i = 10;
+ *out_len = co_data(inc);
+ out->head += *out_len + 10;
return 0;
}
@@ -627,7 +629,7 @@
*/
static int
http_compression_buffer_add_data(struct comp_state *st, struct buffer *in,
- struct buffer *out, int sz)
+ int in_out, struct buffer *out, int sz)
{
int consumed_data = 0;
int data_process_len;
@@ -643,15 +645,15 @@
data_process_len = MIN(b_room(out), sz);
block1 = data_process_len;
- if (block1 > b_contig_data(in, in->o))
- block1 = b_contig_data(in, in->o);
+ if (block1 > b_contig_data(in, in_out))
+ block1 = b_contig_data(in, in_out);
block2 = data_process_len - block1;
/* compressors return < 0 upon error or the amount of bytes read */
- consumed_data = st->comp_algo->add_data(st->comp_ctx, b_peek(in, in->o), block1, out);
+ consumed_data = st->comp_algo->add_data(st->comp_ctx, b_head(in) + in_out, block1, out);
if (consumed_data != block1 || !block2)
goto end;
- consumed_data = st->comp_algo->add_data(st->comp_ctx, in->data, block2, out);
+ consumed_data = st->comp_algo->add_data(st->comp_ctx, b_peek(in, 0), block2, out);
if (consumed_data < 0)
goto end;
consumed_data += block1;
@@ -667,11 +669,12 @@
static int
http_compression_buffer_end(struct comp_state *st, struct stream *s,
struct channel *chn, struct buffer **out,
- int end)
+ unsigned int *buf_out, int end)
{
- struct buffer *ib = chn->buf, *ob = *out;
+ struct buffer *ob = *out;
char *tail;
int to_forward, left;
+ unsigned int tmp_out;
#if defined(USE_SLZ) || defined(USE_ZLIB)
int ret;
@@ -701,35 +704,38 @@
* +---------+---+------------+-----------+
* data p size
*
- * <out> is the room reserved to copy ib->o. It starts at ob->data and
- * has not yet been filled. <c> is the room reserved to write the chunk
- * size (10 bytes). <comp_in> is the compressed equivalent of the data
- * part of ib->i. <empty> is the amount of empty bytes at the end of
- * the buffer, into which we may have to copy the remaining bytes from
- * ib->i after the data (chunk size, trailers, ...).
+ * <out> is the room reserved to copy the channel output. It starts at
+ * ob->data and has not yet been filled. <c> is the room reserved to
+ * write the chunk size (10 bytes). <comp_in> is the compressed
+ * equivalent of the data part of ib->len. <empty> is the amount of
+ * empty bytes at the end of the buffer, into which we may have to
+ * copy the remaining bytes from ib->len after the data
+ * (chunk size, trailers, ...).
*/
/* Write real size at the begining of the chunk, no need of wrapping.
* We write the chunk using a dynamic length and adjust ob->p and ob->i
* accordingly afterwards. That will move <out> away from <data>.
*/
- left = 10 - http_emit_chunk_size(ob->p + 10, ob->i - 10);
- ob->p += left;
- ob->i -= left;
+ left = http_emit_chunk_size(b_head(ob), b_data(ob));
+ b_add(ob, left);
+ ob->head -= *buf_out + (left);
+ /* Copy previous data from chn into ob */
+ if (co_data(chn) > 0) {
+ left = b_contig_data(chn->buf, 0);
+ if (left > *buf_out)
+ left = *buf_out;
- /* Copy previous data from ib->o into ob->o */
- if (ib->o > 0) {
- left = b_contig_data(ib, 0);
- if (left > ib->o)
- left = ib->o;
-
- memcpy(ob->p - ob->o, b_head(ib), left);
- if (ib->o - left) /* second part of the buffer */
- memcpy(ob->p - ob->o + left, ib->data, ib->o - left);
+ memcpy(b_head(ob), co_head(chn), left);
+ b_add(ob, left);
+ if (co_data(chn) - left) {/* second part of the buffer */
+ memcpy(b_head(ob) + left, b_orig(chn->buf), co_data(chn) - left);
+ b_add(ob, co_data(chn) - left);
+ }
}
/* chunked encoding requires CRLF after data */
- tail = ob->p + ob->i;
+ tail = b_tail(ob);
*tail++ = '\r';
*tail++ = '\n';
@@ -751,8 +757,8 @@
}
}
- ob->i = tail - ob->p;
- to_forward = ob->i;
+ b_add(ob, tail - b_tail(ob));
+ to_forward = b_data(ob) - *buf_out;
/* update input rate */
if (st->comp_ctx && st->comp_ctx->cur_lvl > 0) {
@@ -766,19 +772,22 @@
/* copy the remaining data in the tmp buffer. */
c_adv(chn, st->consumed);
- if (ib->i > 0) {
+ if (b_data(chn->buf) - co_data(chn) > 0) {
left = ci_contig_data(chn);
- memcpy(ob->p + ob->i, ci_head(chn), left);
+ memcpy(b_tail(ob), ci_head(chn), left);
b_add(ob, left);
- if (ib->i - left) {
- memcpy(ob->p + ob->i, ib->data, ib->i - left);
- b_add(ob, ib->i - left);
+ if (b_data(chn->buf) - (co_data(chn) + left)) {
+ memcpy(b_tail(ob), b_orig(chn->buf), b_data(chn->buf) - left);
+ b_add(ob, b_data(chn->buf) - left);
}
}
-
/* swap the buffers */
+ *out = chn->buf;
chn->buf = ob;
- *out = ib;
+ tmp_out = chn->buf->output;
+ chn->buf->output = *buf_out;
+ *buf_out = tmp_out;
+
if (st->comp_ctx && st->comp_ctx->cur_lvl > 0) {