The old logic was both too eager to realign (it would move a whole
chain to save a byte) and too reluctant to realign (it would only
realign when data would fit into the misaligned portion, without
considering the space at the end of the chain).
The new logic matches that from evbuffer_expand_singlechain: it only
realigns a chain when not much data is to be moved, and there's a
bunch of space to be regained.
Spotted by Yan Lin.
This change has no effect on non-windows platforms, since those
either define off_t to 64-bits, or allow you to decide whether
it should be 64-bits yourself via some LARGEFILE-like macro.
On Windows, however, off_t is always 32-bit, so it's a bad choice
for "file size" or "file offset" values. Instead, I'm adding
an ev_off_t type, and using it in the one place where we used
off_t to mean "the size of a file" or "an offset into a file" in the
API.
This breaks ABI compatibility on Windows.
The writev() call is limited to at most IOV_MAX iovecs (or UIO_MAXIOV,
depending on whom you ask). This isn't a problem anywhere we've
tested except on OpenSolaris, where IOV_MAX was a mere 16.
This patch makes us go from "use up to 128 iovecs when writing" to
"use up to 128 iovecs when writing, or IOV_MAX/UIO_MAXIOV, whichever
is less". This is still wrong if you somehow find a platform that
defines IOV_MAX < UIO_MAXIOV, but I hereby claim that such a platform
is too stupid to worry about for now.
Found by Michael Herf.
Remember, the code
int is_less_than(int a, unsigned b) {
return a < b;
}
is buggy, since the C integer promotion rules basically turn it into
int is_less_than(int a, unsigned b) {
return ((unsigned)a) < b;
}
and we really want something closer to
int is_less_than(int a, unsigned b) {
return a < 0 || ((unsigned)a) < b;
}
.
Suggested by an example from Ralph Castain
- Prevent evbuffer_{add,prepend}_buffer from moving read-pinned chains.
- Fix evbuffer_drain to handle read-pinned chains better.
- Raise the limit on WSABUFs from two to MAX_WSABUFS for overlapped reads.
The old code had a bug where the 'exact' flag to 1 in
_evbuffer_read_setup_vecs would never actually make the iov_len field
of the last iovec get truncated. This patch fixes that.
The old logic made sense back when buffer.c was an enormous linear
buffer, but it doesn't make any sense for the chain-based
implementation.
This patch also refactors the ioctl{socket}? call into its own function.
For future note, opensolaris doesn't have sys/sysctl.h, doesn't like
comparing iov_buf to a chain_space_ptr without a cast, and is (predictably)
unforgiving of dumb syntax errors.
Also, we had accidentally broken the devpoll backend test in configure.in
The evbuffer_remove() function copies data from the front of an
evbuffer into an array of char, and removes the data from the buffer.
This function behaves the same, but does not remove the data. This
behavior can be handy for lots of protocols, where you want the
evbuffer to accumulate data until a complete record has arrived.
Lots of people have asked for a function more or less like this, and
though it isn't too hard to code one from evbuffer_peek(), it is
apparently annoying to do it in every app you write. The
evbuffer_peek() function is significantly faster, but it requires that
the user be able to handle data in separate extents.
This patch also reimplements evbufer_remove() as evbuffer_copyout()
followed by evbuffer_drain(). I am reasonably confident that this
won't be a performance hit: the memcpy() overhead should dominate the
cost of walking the list an extra time.
The previous evbuffer_expand was not only incorrect; it was
inefficient too. On all questions of time vs memory tradeoffs, it
chose to burn time in order to avoid wasting memory. The new code
tries to be a little more balanced: it only resizes an existing chain
when doing so doesn't require too much copying, and when failing to do
so would waste a lot of the chain's space.
This patch also rewrites evbuffer_chain_insert to work properly with
last_with_datap, and adds a few convenience functions to buffer.c.
Apparently nobody had tested it before on a system that had sendfile.
Why would you have sendfile and not writev? Perhaps you're trying to
test the no-iovecs code to make sure it still works.
To implement evbuffer_expand() properly, you need to be able to
replace the last chunk that has data, which means that we need to keep
track of the the next pointer pointing to the last_with_data chunk,
not the last_with_data chunk itself.
evbuffer_pullup() returns NULL if you try to pull up more bytes than
are there. But evbuffer_write_atmost would sometimes ask for more
bytes to be pulled up than it had, get a NULL, and fail.
If the first chunk of a buffer is empty, and we're told to prepend to
the buffer, we should be willing to use the entire first chunk.
Instead, we were dependent on the value of chunk->misalign.
Previously it would only accept 2 iovecs at most, because our
previous_to_last nonsense didn't let it take any more. This forced us
to do more reallocations in some cases when an extra small malloc
would have sufficed.
This actually makes some of the code a lot simpler. The only
ones that actually used previous_to_last for anything were reserving
and committing space.
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.
This makes some cases of bench_http about 5% faster.
Our internal evbuffer_strpbrk() function was overly general (it tried
to handle all character sets when we only used it for "\r\n"), and
not very efficient (it called memchr once for each character in the
buffer until it found a \r or a \n). It actually showed up in some
profiles for HTTP testing, since evbuffer_readln() calls it when doing
loose CRLF detection. This patch replaces it with a faster
implementation.
This should end the family of bugs where we call bufferevent_free()
while a pending callback is holding a reference on the bufferevent,
and the callback tries to invoke the user callbacks before it releases
its own final reference.
This means that bufferevent_decref() is now a separate function from
bufferevent_free().
Some initializers (in evbuffer_read and evbuffer_commit) were reading
the last and/or previous_to_last fields without grabbing the evbuffer
lock.
This may fix a hard-to-trigger race condition or two.
Previously, our default lock model kind of assumed that every lock was
potentially a read-write lock. This was a poor choice, since
read-write locks are far more expensive than regular locks, and so the
lock API should only use them when we can actually take advantage of
them. Neither our pthreads or win32 lock implementation provided rw
locks.
Now that we have a way (not currently used!) to indicate that we
really want a read-write lock, we shouldn't actually say "lock this
for reading" or "lock this for writing" unless we mean it.
Previously, there was no good way to request different kinds of lock
(say, read/write vs writeonly or recursive vs nonrecursive), or for a
lock function to signal failure (which would be important for a
trylock mode).
This patch revises the lock API to be a bit more useful. The older
lock calls are still supported for now.
We also add a debugging mode to catch common errors in using the
locking APIs.
