diff --git a/CMakeLists.txt b/CMakeLists.txt index 670f3c976..0daef542d 100644 --- 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 index 2f91ddf78..06f15de59 100755 --- 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 index 3af1796b6..97aefc826 100644 --- 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,7 +195,6 @@ if(CHECK_FOUND) add_subdirectory(test) endif(CHECK_FOUND) - if(HAVE_RUST) enable_language(Rust) include(CMakeCargo) @@ -210,7 +202,6 @@ if(HAVE_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 index 68374423c..2d10f64bd 100644 --- a/deps/ccommon/src/cc_bstring.c +++ b/deps/ccommon/src/cc_bstring.c @@ -20,6 +20,8 @@ #include #include +#include + /* * 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 index 0cb1fcc46..25f6a8108 100644 --- 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 index 12cc47c4e..25dbdaeb6 100644 --- 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) {