Add unit tests for buffer, fix buf/dbuf bugs and refactor (twitter#174)

* Add unit tests for buffer, fix buf/dbuf bugs and refactor
  - unit tests for buf/dbuf
  - update api to take const for non mutating function
  - improve option/metric descriptions
  - fix buf_new_cap miscalculation
  - fix memory usage metric on destroy
  - refactor size check for dbuf

include/buffer/cc_buf.h

@@ -38,25 +38,25 @@ extern "C" {
 
 
 /*          name            type                default             description */
-#define BUF_OPTION(ACTION)                                                                              \
-    ACTION( buf_init_size,  OPTION_TYPE_UINT,   BUF_DEFAULT_SIZE,   "default size when buf is created" )\
-    ACTION( buf_poolsize,   OPTION_TYPE_UINT,   BUF_POOLSIZE,       "buf pool size"                    )
+#define BUF_OPTION(ACTION)                                                                       \
+    ACTION( buf_init_size,  OPTION_TYPE_UINT,   BUF_DEFAULT_SIZE,   "init buf size incl header" )\
+    ACTION( buf_poolsize,   OPTION_TYPE_UINT,   BUF_POOLSIZE,       "buf pool size"             )
 
 typedef struct {
     BUF_OPTION(OPTION_DECLARE)
 } buf_options_st;
 
 /*          name              type            description */
-#define BUF_METRIC(ACTION)                                              \
-    ACTION( buf_curr,         METRIC_GAUGE,   "# buf allocated"        )\
-    ACTION( buf_active,       METRIC_GAUGE,   "# buf in use/borrowed"  )\
-    ACTION( buf_create,       METRIC_COUNTER, "# buf creates"          )\
-    ACTION( buf_create_ex,    METRIC_COUNTER, "# buf create exceptions")\
-    ACTION( buf_destroy,      METRIC_COUNTER, "# buf destroys"         )\
-    ACTION( buf_borrow,       METRIC_COUNTER, "# buf borrows"          )\
-    ACTION( buf_borrow_ex,    METRIC_COUNTER, "# buf borrow exceptions")\
-    ACTION( buf_return,       METRIC_COUNTER, "# buf returns"          )\
-    ACTION( buf_memory,       METRIC_GAUGE,   "memory allocated to buf")
+#define BUF_METRIC(ACTION)                                                              \
+    ACTION( buf_curr,         METRIC_GAUGE,   "# buf allocated"                        )\
+    ACTION( buf_active,       METRIC_GAUGE,   "# buf in use/borrowed"                  )\
+    ACTION( buf_create,       METRIC_COUNTER, "# buf creates"                          )\
+    ACTION( buf_create_ex,    METRIC_COUNTER, "# buf create exceptions"                )\
+    ACTION( buf_destroy,      METRIC_COUNTER, "# buf destroys"                         )\
+    ACTION( buf_borrow,       METRIC_COUNTER, "# buf borrows"                          )\
+    ACTION( buf_borrow_ex,    METRIC_COUNTER, "# buf borrow exceptions"                )\
+    ACTION( buf_return,       METRIC_COUNTER, "# buf returns"                          )\
+    ACTION( buf_memory,       METRIC_GAUGE,   "memory alloc'd to buf including header" )
 
 typedef struct {
     BUF_METRIC(METRIC_DECLARE)
@@ -103,7 +103,7 @@ void buf_destroy(struct buf **buf);
 
 /* Size of data that has yet to be read */
 static inline uint32_t
-buf_rsize(struct buf *buf)
+buf_rsize(const struct buf *buf)
 {
     ASSERT(buf->rpos <= buf->wpos);
 
@@ -112,37 +112,38 @@ buf_rsize(struct buf *buf)
 
 /* Amount of room left in buffer for writing new data */
 static inline uint32_t
-buf_wsize(struct buf *buf)
+buf_wsize(const struct buf *buf)
 {
     ASSERT(buf->wpos <= buf->end);
 
     return (uint32_t)(buf->end - buf->wpos);
 }
 
-/* Total capacity of given buf */
+/* Total size of given buf, including header */
 static inline uint32_t
-buf_size(struct buf *buf)
+buf_size(const struct buf *buf)
 {
     ASSERT(buf->begin < buf->end);
 
-    return (uint32_t)(buf->end - (char*)buf);
+    return (uint32_t)(buf->end - (char *)buf);
 }
 
+/* Size of given buf, not including header */
 static inline uint32_t
-buf_capacity(struct buf *buf)
+buf_capacity(const struct buf *buf)
 {
     ASSERT(buf->begin < buf->end);
 
     return (uint32_t)(buf->end - buf->begin);
 }
 
-/* new capacity needed to append count bytes to the buffer */
+/* new capacity needed to write count bytes to the buffer */
 static inline uint32_t
-buf_new_cap(struct buf *buf, uint32_t count)
+buf_new_cap(const struct buf *buf, uint32_t count)
 {
     ASSERT(buf->begin <= buf->wpos);
 
-    return buf->wpos - buf->begin + count;
+    return count <= buf_wsize(buf) ? 0 : count - buf_wsize(buf);
 }
 
 static inline void

include/buffer/cc_dbuf.h

@@ -29,8 +29,8 @@ extern "C" {
 
 
 /*          name                type                default             description */
-#define DBUF_OPTION(ACTION)                                                                         \
-    ACTION( dbuf_max_power,      OPTION_TYPE_UINT,  DBUF_DEFAULT_MAX,   "max number of doubling"   )
+#define DBUF_OPTION(ACTION)                                                                     \
+    ACTION( dbuf_max_power,      OPTION_TYPE_UINT,  DBUF_DEFAULT_MAX,   "max number of doubles")
 
 typedef struct {
     DBUF_OPTION(OPTION_DECLARE)
@@ -43,7 +43,7 @@ typedef struct {
     ACTION( dbuf_double,    METRIC_COUNTER, "# double completed"   )\
     ACTION( dbuf_double_ex, METRIC_COUNTER, "# double failed"      )\
     ACTION( dbuf_shrink,    METRIC_COUNTER, "# shrink completed"   )\
-    ACTION( dbuf_shrink_ex, METRIC_COUNTER, "# srhink failed"      )\
+    ACTION( dbuf_shrink_ex, METRIC_COUNTER, "# shrink failed"      )\
     ACTION( dbuf_fit,       METRIC_COUNTER, "# fit completed"      )\
     ACTION( dbuf_fit_ex,    METRIC_COUNTER, "# fit failed"         )
 

src/buffer/cc_buf.c

@@ -157,8 +157,8 @@ buf_destroy(struct buf **buf)
         return;
     }
 
-    cap = buf_capacity(*buf);
-    log_verb("destroy buf %p capacity %"PRIu32, *buf, cap);
+    cap = buf_size(*buf);
+    log_verb("destroy buf %p size %"PRIu32, *buf, cap);
 
     cc_free(*buf);
     *buf = NULL;

src/buffer/cc_dbuf.c

@@ -51,9 +51,15 @@ static rstatus_i
 _dbuf_resize(struct buf **buf, uint32_t nsize)
 {
     struct buf *nbuf;
-    uint32_t osize = buf_size(*buf);
-    uint32_t roffset = (*buf)->rpos - (*buf)->begin;
-    uint32_t woffset = (*buf)->wpos - (*buf)->begin;
+    uint32_t osize, roffset, woffset;
+
+    if (nsize > max_size) {
+        return CC_ERROR;
+    }
+
+    osize = buf_size(*buf);
+    roffset = (*buf)->rpos - (*buf)->begin;
+    woffset = (*buf)->wpos - (*buf)->begin;
 
     nbuf = cc_realloc(*buf, nsize);
     if (nbuf == NULL) { /* realloc failed, but *buf is still valid */
@@ -80,10 +86,6 @@ dbuf_double(struct buf **buf)
     rstatus_i status;
     uint32_t nsize = buf_size(*buf) * 2;
 
-    if (nsize > max_size) {
-        return CC_ERROR;
-    }
-
     status = _dbuf_resize(buf, nsize);
     if (status == CC_OK) {
         INCR(dbuf_metrics, dbuf_double);
@@ -100,12 +102,14 @@ dbuf_fit(struct buf **buf, uint32_t cap)
     rstatus_i status = CC_OK;
     uint32_t nsize = buf_init_size;
 
-    buf_lshift(*buf);
-    if (buf_rsize(*buf) > cap || cap + BUF_HDR_SIZE > max_size) {
+    /* check if new cap can contain unread bytes */
+    if (buf_rsize(*buf) > cap) {
         return CC_ERROR;
     }
 
-    /* cap is checked, given how max_size is initialized this is safe */
+    buf_lshift(*buf);
+
+    /* double size of buf until it can fit cap */
     while (nsize < cap + BUF_HDR_SIZE) {
         nsize *= 2;
     }
@@ -125,9 +129,9 @@ dbuf_fit(struct buf **buf, uint32_t cap)
 rstatus_i
 dbuf_shrink(struct buf **buf)
 {
-    rstatus_i status = CC_OK;
     uint32_t nsize = buf_init_size;
     uint32_t cap = buf_rsize(*buf);
+    rstatus_i status = CC_OK;
 
     buf_lshift(*buf);
 
@@ -136,7 +140,12 @@ dbuf_shrink(struct buf **buf)
     }
 
     if (nsize != buf_size(*buf)) {
+        /*
+         * realloc is not guaranteed to succeed even on trim, but in the case
+         * that it fails, original buf will still be valid.
+         */
         status = _dbuf_resize(buf, nsize);
+
         if (status == CC_OK) {
             INCR(dbuf_metrics, dbuf_shrink);
         } else {

test/CMakeLists.txt

@@ -4,6 +4,7 @@ add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND})
 
 add_subdirectory(array)
 add_subdirectory(bstring)
+add_subdirectory(buffer)
 add_subdirectory(channel)
 add_subdirectory(event)
 add_subdirectory(log)

test/buffer/CMakeLists.txt

@@ -0,0 +1,10 @@
+set(suite buf)
+set(test_name check_${suite})
+
+set(source check_${suite}.c)
+
+add_executable(${test_name} ${source})
+target_link_libraries(${test_name} ccommon-static ${CHECK_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} m)
+
+add_dependencies(check ${test_name})
+add_test(${test_name} ${test_name})