Revise evbuffer to add last_with_data

This is the first patch in a series to replace previous_to_last with
last_with_data.  Currently, we can only use two partially empty chains
at the end of an evbuffer, so if we have one with 511 bytes free, and
another with 512 bytes free, and we try to do a 1024 byte read, we
can't just stick another chain on the end: we need to reallocate the
last one.  That's stupid and inefficient.

Instead, this patch adds a last_with_data pointer to eventually
replace previous_to_last.  Instead of pointing to the penultimated
chain (if any) as previous_to_last does, last_with_data points to the
last chain that has any data in it, if any.  If all chains are empty,
last_with_data points to the first chain.  If there are no chains,
last_with_data is NULL.

The next step is to start using last_with_data everywhere that we
currently use previous_to_last.  When that's done, we can remove
previous_to_last and the code that maintains it.

buffer.c

@@ -220,9 +220,12 @@ evbuffer_chain_insert(struct evbuffer *buf, struct evbuffer_chain *chain)
 	if (buf->first == NULL) {
 		buf->first = buf->last = chain;
 		buf->previous_to_last = NULL;
+		buf->last_with_data = chain;
 	} else {
 		/* the last chain is empty so we can just drop it */
 		if (buf->last->off == 0 && !CHAIN_PINNED(buf->last)) {
+			if (buf->last_with_data == buf->last)
+				buf->last_with_data = chain;
 			evbuffer_chain_free(buf->last);
 			buf->previous_to_last->next = chain;
 			buf->last = chain;
@@ -231,6 +234,8 @@ evbuffer_chain_insert(struct evbuffer *buf, struct evbuffer_chain *chain)
 			buf->last->next = chain;
 			buf->last = chain;
 		}
+		if (chain->off)
+			buf->last_with_data = chain;
 	}
 
 	buf->total_len += chain->off;
@@ -531,6 +536,22 @@ done:
 
 }
 
+static int
+advance_last_with_data(struct evbuffer *buf)
+{
+	int n = 0;
+	ASSERT_EVBUFFER_LOCKED(buf);
+
+	if (!buf->last_with_data)
+		return 0;
+
+	while (buf->last_with_data->next && buf->last_with_data->next->off) {
+		buf->last_with_data = buf->last_with_data->next;
+		++n;
+	}
+	return n;
+}
+
 int
 evbuffer_commit_space(struct evbuffer *buf,
     struct evbuffer_iovec *vec, int n_vecs)
@@ -578,6 +599,7 @@ evbuffer_commit_space(struct evbuffer *buf,
 	buf->total_len += added;
 	buf->n_add_for_cb += added;
 	result = 0;
+	advance_last_with_data(buf);
 	evbuffer_invoke_callbacks(buf);
 
 done:
@@ -590,6 +612,7 @@ done:
 		(dst)->first = NULL;		\
 		(dst)->last = NULL;		\
 		(dst)->previous_to_last = NULL; \
+		(dst)->last_with_data = NULL;	\
 		(dst)->total_len = 0;		\
 	} while (0)
 
@@ -598,6 +621,7 @@ done:
 		ASSERT_EVBUFFER_LOCKED(src);			   \
 		(dst)->first = (src)->first;			   \
 		(dst)->previous_to_last = (src)->previous_to_last; \
+		(dst)->last_with_data = (src)->last_with_data;	   \
 		(dst)->last = (src)->last;			   \
 		(dst)->total_len = (src)->total_len;		   \
 	} while (0)
@@ -608,6 +632,8 @@ done:
 		(dst)->last->next = (src)->first;			\
 		(dst)->previous_to_last = (src)->previous_to_last ?	\
 		    (src)->previous_to_last : (dst)->last;		\
+		if ((src)->last_with_data)				\
+			(dst)->last_with_data = (src)->last_with_data;	\
 		(dst)->last = (src)->last;				\
 		(dst)->total_len += (src)->total_len;			\
 	} while (0)
@@ -620,9 +646,10 @@ done:
 		(dst)->total_len += (src)->total_len;		   \
 		if ((dst)->previous_to_last == NULL)		   \
 			(dst)->previous_to_last = (src)->last;	   \
+		if ((dst)->last_with_data == NULL)		   \
+			(dst)->last_with_data = (src)->last_with_data; \
 	} while (0)
 
-
 int
 evbuffer_add_buffer(struct evbuffer *outbuf, struct evbuffer *inbuf)
 {
@@ -734,6 +761,8 @@ evbuffer_drain(struct evbuffer *buf, size_t len)
 		for (chain = buf->first; len >= chain->off; chain = next) {
 			next = chain->next;
 			len -= chain->off;
+			if (chain == buf->last_with_data)
+				buf->last_with_data = next;
 
 			if (len == 0 && CHAIN_PINNED_R(chain))
 				break;
@@ -789,6 +818,9 @@ evbuffer_remove(struct evbuffer *buf, void *data_out, size_t datlen)
 		data += chain->off;
 		datlen -= chain->off;
 
+		if (chain == buf->last_with_data)
+			buf->last_with_data = chain->next;
+
 		tmp = chain;
 		chain = chain->next;
 		evbuffer_chain_free(tmp);
@@ -855,6 +887,10 @@ evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst,
 
 	/* removes chains if possible */
 	while (chain->off <= datlen) {
+		/* We can't remove the last with data from src unless we
+		 * remove all chains, in which case we would have done the if
+		 * block above */
+		EVUTIL_ASSERT(chain != src->last_with_data);
 		nread += chain->off;
 		datlen -= chain->off;
 		previous_to_previous = previous;
@@ -871,6 +907,7 @@ evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst,
 		}
 		dst->previous_to_last = previous_to_previous;
 		dst->last = previous;
+		dst->last_with_data = dst->last;
 		previous->next = NULL;
 		src->first = chain;
 		if (src->first == src->last)
@@ -907,6 +944,7 @@ evbuffer_pullup(struct evbuffer *buf, ev_ssize_t size)
 	struct evbuffer_chain *chain, *next, *tmp;
 	unsigned char *buffer, *result = NULL;
 	ev_ssize_t remaining;
+	int removed_last_with_data = 0;
 
 	EVBUFFER_LOCK(buf);
 
@@ -976,6 +1014,8 @@ evbuffer_pullup(struct evbuffer *buf, ev_ssize_t size)
 		memcpy(buffer, chain->buffer + chain->misalign, chain->off);
 		size -= chain->off;
 		buffer += chain->off;
+		if (chain ==  buf->last_with_data)
+			removed_last_with_data = 1;
 
 		evbuffer_chain_free(chain);
 	}
@@ -994,6 +1034,13 @@ evbuffer_pullup(struct evbuffer *buf, ev_ssize_t size)
 
 	tmp->next = chain;
 
+	if (removed_last_with_data) {
+		int n;
+		buf->last_with_data = buf->first;
+		n = advance_last_with_data(buf);
+		EVUTIL_ASSERT(n == 0);
+	}
+
 	result = (tmp->buffer + tmp->misalign);
 
 done:
@@ -1382,6 +1429,11 @@ evbuffer_prepend(struct evbuffer *buf, const void *data, size_t datlen)
 	buf->first = tmp;
 	if (buf->previous_to_last == NULL)
 		buf->previous_to_last = tmp;
+	if (buf->last_with_data == NULL)
+		buf->last_with_data = tmp;
+	else if (chain && buf->last_with_data == chain && 0==chain->off)
+		buf->last_with_data = tmp;
+
 	tmp->next = chain;
 
 	tmp->off = datlen;
@@ -1465,6 +1517,8 @@ evbuffer_expand(struct evbuffer *buf, size_t datlen)
 	if (buf->previous_to_last)
 		buf->previous_to_last->next = tmp;
 	buf->last = tmp;
+	if (buf->last->off || buf->last_with_data == chain)
+		buf->last_with_data = tmp;
 
 	evbuffer_chain_free(chain);
 
@@ -1528,8 +1582,9 @@ _evbuffer_expand_fast(struct evbuffer *buf, size_t datlen)
 		if (buf->previous_to_last)
 			buf->previous_to_last->next = tmp;
 		buf->last = tmp;
+		if (chain == buf->last_with_data)
+			buf->last_with_data = tmp;
 		evbuffer_chain_free(chain);
-
 	} else {
 		/* Add a new chunk big enough to hold what won't fit
 		 * in chunk. */

evbuffer-internal.h

@@ -84,6 +84,10 @@ struct evbuffer {
 	 * repurposed accordingly. */
 	struct evbuffer_chain *previous_to_last;
 
+	/** The last chain that has any data in it.  If all chains in the
+	 * buffer are empty, points to the first chain */
+	struct evbuffer_chain *last_with_data;
+
 	/** Total amount of bytes stored in all chains.*/
 	size_t total_len;
 

test/regress_buffer.c

@@ -68,6 +68,7 @@ _evbuffer_validate(struct evbuffer *buf)
 {
 	struct evbuffer_chain *chain, *previous = NULL;
 	size_t sum = 0;
+	int found_last_with_data = 0;
 
 	if (buf->first == NULL) {
 		tt_assert(buf->last == NULL);
@@ -81,6 +82,8 @@ _evbuffer_validate(struct evbuffer *buf)
 
 	chain = buf->first;
 	while (chain != NULL) {
+		if (chain == buf->last_with_data)
+			found_last_with_data = 1;
 		sum += chain->off;
 		if (chain->next == NULL) {
 			tt_assert(buf->previous_to_last == previous);
@@ -91,6 +94,23 @@ _evbuffer_validate(struct evbuffer *buf)
 		chain = chain->next;
 	}
 
+	if (buf->first)
+		tt_assert(buf->last_with_data);
+	if (buf->last_with_data) {
+		tt_assert(found_last_with_data);
+		chain = buf->last_with_data;
+		if (chain->off == 0 || buf->total_len == 0) {
+			tt_assert(chain->off == 0)
+			tt_assert(chain == buf->first);
+			tt_assert(buf->total_len == 0);
+		}
+		chain = chain->next;
+		while (chain != NULL) {
+			tt_assert(chain->off == 0);
+			chain = chain->next;
+		}
+	}
+
 	tt_assert(sum == buf->total_len);
 	return 1;
  end:
@@ -239,8 +259,10 @@ test_evbuffer_reserve2(void *ptr)
 	cp = v[0].iov_base;
 	remaining = v[0].iov_len - 512;
 	v[0].iov_len = 512;
+	evbuffer_validate(buf);
 	tt_int_op(0, ==, evbuffer_commit_space(buf, v, 1));
 	tt_int_op(evbuffer_get_length(buf), ==, 512);
+	evbuffer_validate(buf);
 
 	/* Ask for another same-chunk request, in an existing chunk. Use 8
 	 * bytes of it. */
@@ -253,6 +275,7 @@ test_evbuffer_reserve2(void *ptr)
 	tt_int_op(0, ==, evbuffer_commit_space(buf, v, 1));
 	tt_int_op(evbuffer_get_length(buf), ==, 520);
 	remaining -= 8;
+	evbuffer_validate(buf);
 
 	/* Now ask for a request that will be split. Use only one byte of it,
 	   though. */
@@ -268,6 +291,7 @@ test_evbuffer_reserve2(void *ptr)
 	tt_int_op(0, ==, evbuffer_commit_space(buf, v, 1));
 	tt_int_op(evbuffer_get_length(buf), ==, 521);
 	remaining -= 1;
+	evbuffer_validate(buf);
 
 	/* Now ask for a request that will be split. Use some of the first
 	 * part and some of the second. */
@@ -283,7 +307,7 @@ test_evbuffer_reserve2(void *ptr)
 	v[1].iov_len = 60;
 	tt_int_op(0, ==, evbuffer_commit_space(buf, v, 2));
 	tt_int_op(evbuffer_get_length(buf), ==, 981);
-
+	evbuffer_validate(buf);
 
 	/* Now peek to make sure stuff got made how we like. */
 	memset(v,0,sizeof(v));
@@ -358,6 +382,7 @@ test_evbuffer_reference(void *ptr)
 
 	tt_assert(!memcmp(evbuffer_pullup(dst, strlen(data)),
 			  data, strlen(data)));
+	evbuffer_validate(dst);
 
  end:
 	evbuffer_free(dst);
@@ -394,6 +419,7 @@ test_evbuffer_add_file(void *ptr)
 	if (memcmp(compare, data, strlen(data)))
 		tt_abort_msg("Data from add_file differs.");
 
+	evbuffer_validate(src);
  end:
 	EVUTIL_CLOSESOCKET(pair[0]);
 	EVUTIL_CLOSESOCKET(pair[1]);
@@ -711,6 +737,7 @@ test_evbuffer_ptr_set(void *ptr)
 	v[0].iov_len = 5000;
 	memset(v[0].iov_base, 1, v[0].iov_len);
 	evbuffer_commit_space(buf, v, 1);
+	evbuffer_validate(buf);
 
 	evbuffer_reserve_space(buf, 4000, v, 1);
 	v[0].iov_len = 4000;
@@ -721,6 +748,7 @@ test_evbuffer_ptr_set(void *ptr)
 	v[0].iov_len = 3000;
 	memset(v[0].iov_base, 3, v[0].iov_len);
 	evbuffer_commit_space(buf, v, 1);
+	evbuffer_validate(buf);
 
 	tt_int_op(evbuffer_get_length(buf), ==, 12000);
 
@@ -867,7 +895,9 @@ test_evbuffer_callbacks(void *ptr)
 	evbuffer_add_printf(buf, "This will not.");
 	tt_str_op(evbuffer_pullup(buf, -1), ==, "This will not.");
 
+	evbuffer_validate(buf);
 	evbuffer_drain(buf, evbuffer_get_length(buf));
+	evbuffer_validate(buf);
 
 #if 0
 	/* Now let's try a suspended callback. */
@@ -938,6 +968,7 @@ test_evbuffer_add_reference(void *ptr)
 	/* Make sure that prepending does not meddle with immutable data */
 	tt_int_op(evbuffer_prepend(buf1, "I have ", 7), ==, 0);
 	tt_int_op(memcmp(chunk1, "If you", 6), ==, 0);
+	evbuffer_validate(buf1);
 
 	/* Make sure that when the chunk is over, the callback is invoked. */
 	evbuffer_drain(buf1, 7); /* Remove prepended stuff. */
@@ -949,6 +980,7 @@ test_evbuffer_add_reference(void *ptr)
 	tt_assert(ref_done_cb_called_with_data == chunk1);
 	tt_assert(ref_done_cb_called_with_len == len1);
 	tt_int_op(ref_done_cb_called_count, ==, 1);
+	evbuffer_validate(buf1);
 
 	/* Drain some of the remaining chunk, then add it to another buffer */
 	evbuffer_drain(buf1, 6); /* Remove the ", you ". */
@@ -964,6 +996,7 @@ test_evbuffer_add_reference(void *ptr)
 	evbuffer_drain(buf2, evbuffer_get_length(buf2));
 	tt_int_op(ref_done_cb_called_count, ==, 2);
 	tt_assert(ref_done_cb_called_with == (void*)222);
+	evbuffer_validate(buf2);
 
 	/* Now add more stuff to buf1 and make sure that it gets removed on
 	 * free. */