Ccommon update & finding check in cmake (twitter#257)

* Squashed 'deps/ccommon/' changes from f5efe29..4e99e63 4e99e63 fix a bug and change how check is found (twitter#214) 54067ef slightly simplify accept error-handling logic (twitter#210) e9fe980 Fix synchronize ccommon with pelikan deps/ccommon (twitter#212) 7eb6424 Cleanup libcheck related code (twitter#211) 683bc1a cc_bstring simplify and fix (twitter#207) 8737d99 continue on server socket on non-blocking errors (twitter#209) 2a62281 add atoi64 to bstring (twitter#206) f71c657 cc_option simplify _allowed_in_name (twitter#205) 24e3131 Add ITT instrumentation option (twitter#204) 236c98d Fix docs (twitter#200) e58f6a8 cc_array and cc_ring_array NULL fixes (twitter#201) 1c8df42 Add basic support of build type (twitter#199) 7107988 Fix now_ns() (twitter#198) da240e5 cc: extend cc_util module (twitter#196) 4846b15 Fix TAILQ_REINIT (twitter#195) 4f5dbb0 Update Cmake version to 2.8 (twitter#197) 2e6f78a cc_mm use OS_DARWIN macro to detect OS (twitter#194) 57acaf6 cc: extend queue module (twitter#193) a64ada2 cc: extend duration module (twitter#192) b117632 reverting CMake file changes (twitter#191) dea5bee backport changes made to ccommon in pelikan (twitter#190) a4c0334 add linebreak to stats_log() (twitter#188) 05eb03e fix inconsistent naming and bump version (twitter#187) 4acc53a Stats to file (twitter#186) 2168fec minimize osx build config (twitter#185) 42b24de Simplify rust options, specify fewer output targets (twitter#183) c9fa905 update CMakeRust used to latest version, tweaks to make build work (twitter#184) 2ef0163 Reorder dependency includes in cmake, don't parallel build (twitter#182) a6a54d9 remove endian-specific logic from str*cmp (twitter#177) 4c0668b epoll_create* ignores size hint in newer kernels, switch to new API (twitter#179) c9c5ee5 improve cc_bstring string literal and cstring names (twitter#176) 0184d73 Add unit tests for buffer, fix buf/dbuf bugs and refactor (twitter#174) d7dab43 create a .cargo/config so intellij uses the same target dir as cmake (twitter#173) e710712 use accept4 for tcp_accept when available (twitter#171) 21ba10e Remove cargo lock for shared lib, closes twitter#169 (twitter#172) 24660f1 update style guide (twitter#170) 17baf1e Per thread logging (twitter#168) git-subtree-dir: deps/ccommon git-subtree-split: 4e99e63 * fix check linkage problem * temp * missed one conflict * fix ci/run.sh * fix ci/run.sh more
michalbiesek · Sep 13, 2019 · 237a7dd · 237a7dd
1 parent 683fc51
commit 237a7dd
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 55 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -116,11 +116,6 @@ if(BUILD_AND_INSTALL_CHECK)
     set(CMAKE_REQUIRED_INCLUDES "${CHECK_ROOT_DIR}/include")    # these make check link correctly in ccommon and pelikan
 endif()
 
-find_package(PkgConfig QUIET)
-
-if(PKG_CONFIG_FOUND)
-    pkg_check_modules(CHECK QUIET check>=0.10)
-endif()
 
 if(${OS_PLATFORM} MATCHES "OS_LINUX")
   set(CFLAGS_LIST "${CFLAGS_LIST} -lrt")
@@ -146,9 +141,7 @@ add_subdirectory(${CCOMMON_SOURCE_DIR} ${PROJECT_BINARY_DIR}/ccommon)
 include(FindPackageHandleStandardArgs)
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PROJECT_SOURCE_DIR}/cmake")
 
-if(NOT CHECK_FOUND)
-    find_package(Check QUIET 0.10)
-endif()
+find_package(PkgConfig QUIET)
 
 if (USE_PMEM)
     if(PKG_CONFIG_FOUND)
@@ -170,6 +163,17 @@ if (HAVE_ITT_INSTRUMENTATION)
     link_libraries(${ITTNOTIFY_LIBRARIES})
 endif(HAVE_ITT_INSTRUMENTATION)
 
+find_package(Check)
+if(NOT CHECK_FOUND)
+    message(WARNING "Check is required to build and run tests")
+endif(NOT CHECK_FOUND)
+if(CHECK_FOUND)
+    check_symbol_exists(ck_assert_int_eq check.h CHECK_WORKING)
+    if(NOT CHECK_WORKING)
+        message(WARNING "Check version too old to build tests")
+    endif(NOT CHECK_WORKING)
+endif(CHECK_FOUND)
+
 find_package(Threads)
 
 if(TARGET_CDB)

diff --git a/ci/run.sh b/ci/run.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-set -euo pipefail
+set -uo pipefail
 IFS=$'\n\t'
 
 die() { echo "fatal: $*" >&2; exit 1; }
@@ -11,19 +11,20 @@ trap cleanup EXIT
 
 export PATH=$HOME/.cargo/bin:$PATH
 
-# build CDB in CI or else stuff breaks
-
-CMAKE_ARGS=(
+# TODO: run cmake3 on centos hosts
+cmake_cmd=(
+  cmake
   -DBUILD_AND_INSTALL_CHECK=yes
 )
 
+# build CDB in CI or else stuff breaks
+
 if [[ -n "${RUST_ENABLED:-}" ]]; then
-  CMAKE_ARGS+=( -DTARGET_CDB=yes -DHAVE_RUST=yes -DRUST_VERBOSE_BUILD=1 )
+  cmake_cmd+=( -DTARGET_CDB=yes -DHAVE_RUST=yes -DRUST_VERBOSE_BUILD=yes )
 fi
 
-# TODO: run cmake3 on centos hosts
 
-mkdir -p _build && ( cd _build && cmake ${CMAKE_ARGS[@]} .. && make && make test ) || die 'make failed'
+mkdir -p _build && ( cd _build && "${cmake_cmd[@]}" .. && make && make test ) || die 'make failed'
 
 egrep -r ":F:|:E:" . |grep -v 'Binary file' || true
 

diff --git a/deps/ccommon/CMakeLists.txt b/deps/ccommon/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 2.6)
+cmake_minimum_required(VERSION 2.8)
 project(ccommon C)
 
 # Uncomment the following to output dependency graph debugging messages
@@ -115,7 +115,6 @@ configure_file(
 # set compiler flags
 # string concat is easier in 3.0, but older versions don't have the concat subcommand
 # so we are using list as input until we move to new version
-
 add_definitions(-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64)
 # Set a default build type (Release) if none was specified
 
@@ -128,6 +127,7 @@ if(CMAKE_BUILD_TYPE MATCHES Debug)
 else()
     set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O2")
 endif()
+
 set(CMAKE_MACOSX_RPATH 1)
 set(CFLAGS_LIST
     "-std=c11 "
@@ -153,23 +153,16 @@ endif(COVERAGE)
 # test dependencies
 include(FindPackageHandleStandardArgs)
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PROJECT_SOURCE_DIR}/cmake")
-
-find_package(PkgConfig QUIET)
-
+find_package(Check)
 if(NOT CHECK_FOUND)
-    find_package(Check QUIET 0.10)
-endif()
-
-if (HAVE_ITT_INSTRUMENTATION)
-    if(PKG_CONFIG_FOUND)
-        pkg_check_modules(ITTNOTIFY REQUIRED ittnotify>=1.0)
-    else()
-        find_package(ITTNOTIFY REQUIRED 1.0)
-    endif()
-    include_directories(${ITTNOTIFY_INCLUDE_DIRS})
-    link_directories(${ITTNOTIFY_LIBRARY_DIRS})
-    link_libraries(${ITTNOTIFY_LIBRARIES})
-endif(HAVE_ITT_INSTRUMENTATION)
+    message(WARNING "Check is required to build and run tests")
+endif(NOT CHECK_FOUND)
+if(CHECK_FOUND)
+    check_symbol_exists(ck_assert_int_eq check.h CHECK_WORKING)
+    if(NOT CHECK_WORKING)
+        message(WARNING "Check version too old to build tests")
+    endif(NOT CHECK_WORKING)
+endif(CHECK_FOUND)
 
 if (HAVE_ITT_INSTRUMENTATION)
     if(PKG_CONFIG_FOUND)
@@ -202,15 +195,13 @@ if(CHECK_FOUND)
     add_subdirectory(test)
 endif(CHECK_FOUND)
 
-
 if(HAVE_RUST)
     enable_language(Rust)
     include(CMakeCargo)
     add_subdirectory(rust)
     set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -DHAVE_RUST=1")
 endif()
 
-
 ###################
 # print a summary #
 ###################

diff --git a/deps/ccommon/src/cc_bstring.c b/deps/ccommon/src/cc_bstring.c
@@ -20,6 +20,8 @@
 #include <cc_debug.h>
 #include <cc_mm.h>
 
+#include <ctype.h>
+
 /*
  * Byte string (struct bstring) is a sequence of unsigned char
  * The length of the string is pre-computed and explicitly available.
@@ -131,14 +133,14 @@ bstring_atoi64(int64_t *i64, struct bstring *str)
 
     for (*i64 = 0LL; offset < str->len; offset++) {
         c = *(str->data + offset);
-        if (c < '0' || c > '9') {
+        if (isdigit(c) == 0) {
             return CC_ERROR;
         }
 
         // overflow check
         if (offset == CC_INT64_MAXLEN - 2) {
             if (sign < 0 && *i64 == INT64_MIN / 10 &&
-                    c - '0' > (uint64_t)(-INT64_MIN) % 10) {
+                    c - '0' > -(INT64_MIN % 10)) {
                 return CC_ERROR;
             }
             if (sign > 0 && *i64 == INT64_MAX / 10 &&
@@ -167,7 +169,7 @@ bstring_atou64(uint64_t *u64, struct bstring *str)
 
     for (offset = 0; offset < str->len; offset++) {
         c = *(str->data + offset);
-        if (c < '0' || c > '9') {
+        if (isdigit(c) == 0) {
             return CC_ERROR;
         }
 

diff --git a/deps/ccommon/src/channel/cc_tcp.c b/deps/ccommon/src/channel/cc_tcp.c
@@ -319,33 +319,63 @@ _tcp_accept(struct tcp_conn *sc)
 {
     int sd;
 
-    ASSERT(sc->sd > 0);
-
-    for (;;) { /* we accept at most one tcp_conn with the 'break' at the end */
+    ASSERT(sc->sd >= 0);
+
+    /* How does tcp accept work when a separate thread is used to accept new
+     * connections?
+     *
+     * In general, we want to accept a new connection at a time (on the server
+     * thread), then hand this connection over to be put on some other event
+     * loop (e.g. on a worker thread's), and some additional preparation may
+     * be necessary (e.g. allocating R/W buffers). This is why we break after
+     * completing a single `accept' successfully.
+     *
+     * There are several ways `accept' could "fail", and they need to be
+     * treated differently. The most common case, which isn't really a failure
+     * on a non-blocking socket, is receiving EAGAIN/EWOULDBLOCK. This simply
+     * means there is no new connection to accept and the function should
+     * return.
+     * EINTR is another common error which means the call was terminated by
+     * a signal. This type of interruption is almost always transient, so
+     * an immediate retry is likely to succeed.
+     * The rest of the exceptions likely to occur on a SOCK_STREAM socket are
+     * often due to exhaustion of some resources (e.g. fd, memory), and there
+     * is no guarantee they will recover immediately. For example, to free
+     * up another fd requires an existing connection to be closed. In such
+     * cases, the connection in the backlog will sit there (as fully
+     * established as far as TCP stack is concerned) until accept in application
+     * becomes possible again, and any new connections arriving will be added
+     * to the back of the queue until it's full, at which point the client
+     * will receive an exception and the connect attempt will fail.
+     */
+    for (;;) {
 #ifdef CC_ACCEPT4
         sd = accept4(sc->sd, NULL, NULL, SOCK_NONBLOCK);
 #else
         sd = accept(sc->sd, NULL, NULL);
 #endif /* CC_ACCEPT4 */
         if (sd < 0) {
-            if (errno == EINTR) {
-                log_debug("accept on sd %d not ready: eintr", sc->sd);
-                continue;
-            }
-
             if (errno == EAGAIN || errno == EWOULDBLOCK) {
                 log_debug("accept on sd %d not ready: eagain", sc->sd);
                 return -1;
             }
 
+            if (errno == EINTR) {
+                log_debug("accept on sd %d not ready: eintr", sc->sd);
+
+                continue;
+            }
+
             log_error("accept on sd %d failed: %s", sc->sd, strerror(errno));
             INCR(tcp_metrics, tcp_accept_ex);
-            continue;
+
+            return -1;
         }
 
         break;
     }
 
+    ASSERT(sd >= 0);
     return sd;
 }
 
@@ -423,18 +453,20 @@ tcp_reject_all(struct tcp_conn *sc)
     for (;;) {
         sd = accept(sc->sd, NULL, NULL);
         if (sd < 0) {
-            if (errno == EINTR) {
-                log_debug("sd %d not ready: eintr", sc->sd);
-                continue;
-            }
-
             if (errno == EAGAIN || errno == EWOULDBLOCK) {
                 log_debug("sd %d has no more outstanding connections", sc->sd);
                 return;
             }
 
+            if (errno == EINTR) {
+                log_debug("accept on sd %d not ready: eintr", sc->sd);
+
+                continue;
+            }
+
             log_error("accept on sd %d failed: %s", sc->sd, strerror(errno));
             INCR(tcp_metrics, tcp_reject_ex);
+
             return;
         }
 

diff --git a/deps/ccommon/src/event/cc_epoll.c b/deps/ccommon/src/event/cc_epoll.c
@@ -122,9 +122,6 @@ _event_update(struct event_base *evb, int fd, int op, uint32_t events, void *ptr
 {
     struct epoll_event event;
 
-    ASSERT(evb != NULL && evb->ep > 0);
-    ASSERT(fd > 0);
-
     event.events = events;
     event.data.ptr = ptr;
 
@@ -135,6 +132,9 @@ int event_add_read(struct event_base *evb, int fd, void *data)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     /*
      * Note(yao): there have been tests showing EPOLL_CTL_ADD is cheaper than
      * EPOLL_CTL_MOD, and the only difference is we need to ignore EEXIST
@@ -156,6 +156,9 @@ event_add_write(struct event_base *evb, int fd, void *data)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     status = _event_update(evb, fd, EPOLL_CTL_ADD, EPOLLOUT, data);
     if (status < 0 && errno != EEXIST) {
         log_error("ctl (add write) w/ epoll fd %d on fd %d failed: %s", evb->ep,
@@ -173,6 +176,9 @@ event_del(struct event_base *evb, int fd)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     /* event can be NULL in kernel >=2.6.9, here we keep it for compatibility */
     status = _event_update(evb, fd, EPOLL_CTL_DEL, 0, NULL);
     if (status < 0) {