r18486@catbus: nickm | 2008-02-28 13:35:53 -0500

 Make offsetof into evutil_offsetof.  Be a little more willing to call evbuffer_chain_align() from evbuffer_expand().  Clarify some docs, and add some XXX comments to note questionable areas.

svn:r677

diff --git a/buffer.c b/buffer.c
index 2b5a825572e0678146f60e7169cd31cd8bd65f2d..41d4bc2779cb4a24a70d2811c2b159292932b3e5 100644 (file)
--- a/buffer.c
+++ b/buffer.c
@@ -330,6 +330,7 @@ evbuffer_remove_buffer(struct evbuffer *src, struct evbuffer *dst,
        return (nread);
 }
 
+/* XXX shouldn't the second arg be ssize_t? */
 u_char *
 evbuffer_pullup(struct evbuffer *buf, int size)
 {
@@ -338,12 +339,17 @@ evbuffer_pullup(struct evbuffer *buf, int size)
 
        if (size == -1)
                size = buf->total_len;
+       /* XXX Does it make more sense, if size > buf->total_len, to
+        * clip it downwards? */
        if (size == 0 || size > buf->total_len)
                return (NULL);
 
+       /* No need to pull up anything; the first size bytes are already here. */
        if (chain->off >= size)
                return chain->buffer + chain->misalign;
 
+       /* XXX is it possible that buf->chain will already have enough space
+        * to hold size bytes? */
        if ((tmp = evbuffer_chain_new(size)) == NULL) {
                event_warn("%s: out of memory\n", __func__);
                return (NULL);
@@ -352,6 +358,7 @@ evbuffer_pullup(struct evbuffer *buf, int size)
        tmp->off = size;
        buffer = tmp->buffer;
 
+       /* Copy and free every chunk that will be entirely pulled into tmp */
        for (chain = buf->first;
            chain != NULL && size >= chain->off; chain = next) {
                next = chain->next;
@@ -563,6 +570,8 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen)
        const u_char *data = data_in;
        size_t old_len = buf->total_len, remain, to_alloc;
 
+       /* If there are no chains allocated for this buffer, allocate one
+        * big enough to hold all the data. */
        if (chain == NULL) {
                if (evbuffer_expand(buf, datlen) == -1)
                        return (-1);
@@ -571,7 +580,7 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen)
 
        remain = chain->buffer_len - chain->misalign - chain->off;
        if (remain >= datlen) {
-               /* we have enough space */
+               /*there's enough space to hold all the data in the current last chain*/
                memcpy(chain->buffer + chain->misalign + chain->off,
                    data, datlen);
                chain->off += datlen;
@@ -580,6 +589,7 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen)
        }
 
        /* we need to add another chain */
+       /* XXX Does this double the length of every successive chain? */
        to_alloc = chain->buffer_len << 1;
        if (datlen > to_alloc)
                to_alloc = datlen;
@@ -592,7 +602,7 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen)
        memcpy(chain->buffer + chain->misalign + chain->off,
            data, remain);
        chain->off += remain;
-               
+
        data += remain;
        datlen -= remain;
 
@@ -628,6 +638,9 @@ evbuffer_prepend(struct evbuffer *buf, const void *data, size_t datlen)
                chain->misalign -= datlen;
        } else {
                struct evbuffer_chain *tmp;
+               /* XXX we should copy as much of the data into chain as possible
+                * before we put any into tmp. */
+
                /* we need to add another chain */
                if ((tmp = evbuffer_chain_new(datlen)) == NULL)
                        return (-1);
@@ -647,6 +660,8 @@ evbuffer_prepend(struct evbuffer *buf, const void *data, size_t datlen)
        return (0);
 }
 
+/** Helper: realigns the memory in chain->buffer so that misalign is
+ * 0. */
 static void
 evbuffer_chain_align(struct evbuffer_chain *chain)
 {
@@ -677,16 +692,16 @@ evbuffer_expand(struct evbuffer *buf, size_t datlen)
        if (chain->buffer_len >= need)
                return (0);
 
-       /*
-        * If the misalignment fulfills our data needs, we just force an
-        * alignment to happen.  Afterwards, we have enough space.
+       /* If the misalignment plus the remaining space fulfils our data needs, we
+        * just force an alignment to happen.  Afterwards, we have enough space.
         */
-       if (chain->misalign >= datlen) {
+       if (chain->buffer_len - chain->off >= datlen) {
                evbuffer_chain_align(chain);
                return (0);
        }
 
-       /* avoid a memcpy if we can just a new chain */
+       /* avoid a memcpy if we can just append a new chain */
+       /* XXX in practice, does this result in lots of leftover space?  */
        length = chain->buffer_len << 1;
        if (length < datlen)
                length = datlen;
@@ -734,11 +749,13 @@ evbuffer_read(struct evbuffer *buf, evutil_socket_t fd, int howmuch)
                else if (n > chain->buffer_len << 2)
                        n = chain->buffer_len << 2;
        }
-#endif 
+#endif
        if (howmuch < 0 || howmuch > n)
                howmuch = n;
 
        /* If we don't have FIONREAD, we might waste some space here */
+       /* XXX we _will_ waste some space here if there is any space left
+        * over on buf->last. */
        if (evbuffer_expand(buf, howmuch) == -1)
                return (-1);
 
@@ -777,6 +794,9 @@ evbuffer_write(struct evbuffer *buffer, evutil_socket_t fd)
        struct iovec iov[NUM_IOVEC];
        struct evbuffer_chain *chain = buffer->first;
        int i = 0;
+       /* XXX make this top out at some maximal data length? if the buffer has
+        * (say) 1MB in it, split over 128 chains, there's no way it all gets
+        * written in one go. */
        while (chain != NULL && i < NUM_IOVEC) {
                iov[i].iov_base = chain->buffer + chain->misalign;
                iov[i++].iov_len = chain->off;
@@ -885,7 +905,8 @@ evbuffer_add_printf(struct evbuffer *buf, const char *fmt, ...)
        return (res);
 }
 
-void evbuffer_setcb(struct evbuffer *buffer,
+void
+evbuffer_setcb(struct evbuffer *buffer,
     void (*cb)(struct evbuffer *, size_t, size_t, void *),
     void *cbarg)
 {

diff --git a/configure.in b/configure.in
index fa37e13a968406489a1a00ed16633965f8948742..e6582a0356fd2c16d71712bf9dc16b44b2b7022e 100644 (file)
--- a/configure.in
+++ b/configure.in
@@ -39,7 +39,7 @@ AC_CHECK_LIB(nsl, inet_ntoa)
 
 dnl Checks for header files.
 AC_HEADER_STDC
-AC_CHECK_HEADERS(fcntl.h stdarg.h inttypes.h stdint.h poll.h signal.h unistd.h sys/epoll.h sys/time.h sys/queue.h sys/event.h sys/param.h sys/ioctl.h sys/select.h sys/devpoll.h port.h netinet/in6.h sys/socket.h sys/uio.h)
+AC_CHECK_HEADERS(fcntl.h stdarg.h inttypes.h stdint.h stddef.h poll.h signal.h unistd.h sys/epoll.h sys/time.h sys/queue.h sys/event.h sys/param.h sys/ioctl.h sys/select.h sys/devpoll.h port.h netinet/in6.h sys/socket.h sys/uio.h)
 if test "x$ac_cv_header_sys_queue_h" = "xyes"; then
        AC_MSG_CHECKING(for TAILQ_FOREACH in sys/queue.h)
        AC_EGREP_CPP(yes,

diff --git a/evbuffer-internal.h b/evbuffer-internal.h
index 25901da1f807a68db502cb1d0d5c8812cfbf648d..5df1b1a2ceb9ef8d634c4a6e0c45f3eb0b863536 100644 (file)
--- a/evbuffer-internal.h
+++ b/evbuffer-internal.h
@@ -32,6 +32,7 @@ extern "C" {
 #endif
 
 #include "config.h"
+#include "evutil.h"
 
 /* minimum allocation */
 #define MIN_BUFFER_SIZE        256
@@ -55,18 +56,16 @@ struct evbuffer_chain {
        /** points to next buffer in the chain */
        struct evbuffer_chain *next;
        
-       size_t buffer_len;
+       size_t buffer_len; /**< total allocation available in the buffer field. */
 
-       size_t misalign;/** unused space at the beginning of buffer */
-       size_t off;     /** write pointer into buffer + misalign */
+       size_t misalign; /**< unused space at the beginning of buffer */
+       size_t off;     /**< Offset into buffer + misalign at which to start writing.
+                                * In other words, the total number of bytes actually stored
+                                * in buffer. */
 
        u_char buffer[1];
 };
 
-#ifndef offsetof
-#define offsetof(type, field) ((size_t)(&((type *)0)->field))
-#endif
-
 #define EVBUFFER_CHAIN_SIZE offsetof(struct evbuffer_chain, buffer[0])
 
 #ifdef __cplusplus

diff --git a/evutil.h b/evutil.h
index 5429501477dbb7cea1838990bd0ff152a5d9802a..d597db0ab582d0251222576b0348d888d36ca0dd 100644 (file)
--- a/evutil.h
+++ b/evutil.h
@@ -50,6 +50,9 @@ extern "C" {
 #ifdef _EVENT_HAVE_SYS_TYPES_H
 #include <sys/types.h>
 #endif
+#ifdef _EVENT_HAVE_STDDEF_H
+#include <stddef.h>
+#endif
 
 #ifdef _EVENT_HAVE_UINT64_T
 #define ev_uint64_t uint64_t
@@ -171,6 +174,12 @@ int evutil_make_socket_nonblocking(evutil_socket_t sock);
 #define        evutil_timerisset(tvp)  ((tvp)->tv_sec || (tvp)->tv_usec)
 #endif
 
+#ifdef offsetof
+#define evutil_offsetof(type, field) offsetof(type, field)
+#else
+#define evutil_offsetof(type, field) ((off_t)(&((type *)0)->field))
+#endif
+
 /* big-int related functions */
 ev_int64_t evutil_strtoll(const char *s, char **endptr, int base);
 

diff --git a/include/event2/buffer.h b/include/event2/buffer.h
index 7cd631f0e7872410b7a72311ec52460e4e071621..09276538ebd06b166987b3e3494622544f2ca739 100644 (file)
--- a/include/event2/buffer.h
+++ b/include/event2/buffer.h
@@ -95,7 +95,8 @@ size_t evbuffer_length(struct evbuffer *buf);
 /**
   Expands the available space in an event buffer.
 
-  Expands the available space in the event buffer to at least datlen
+  Expands the available space in the event buffer to at least datlen, so that
+  appending datlen additional bytes will not require any new allocations.
 
   @param buf the event buffer to be expanded
   @param datlen the new minimum length requirement
@@ -271,10 +272,11 @@ void evbuffer_setcb(struct evbuffer *buffer,
     void (*cb)(struct evbuffer *, size_t, size_t, void *), void *cbarg);
 
 /**
-  Makes the memory in an evbuffer contiguous
+  Makes the memory at the begging of an evbuffer contiguous
 
   @param buf the evbuffer to make contiguous
-  @param size the number of bytes to make contiguous
+  @param size the number of bytes to make contiguous, or -1 to make the
+         entire buffer contiguous.
   @return a pointer to the contigous memory areay
 */
 
@@ -300,6 +302,18 @@ int evbuffer_prepend(struct evbuffer *buf, const void *data, size_t size);
 
 void evbuffer_prepend_buffer(struct evbuffer *dst, struct evbuffer* src);
 
+/* XXX missing APIs:
+
+    A better find-string that returns a smart offset structure rather than a
+    pointer. It should also be able to start searching _at_ an offset.
+
+       A variant of evbuffer_write() that takes a maximum number of bytes to
+       write.
+
+       A check-representation functions for testing, so we can assert() that
+       nothing has gone screwy inside an evbuffer.
+*/
+
 /** deprecated in favor of calling the functions directly */
 #define EVBUFFER_LENGTH(x)     evbuffer_length(x)
 #define EVBUFFER_DATA(x)       evbuffer_pullup(x, -1)