From 3cc374456bc9428fac3ce95203d15ebbe393c09c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 8 Jun 2016 17:29:06 -0400 Subject: [PATCH 1/2] Add several test scripts wrapping test_workqueue This is a fairly easy way for us to get our test coverage up on compat_threads.c and workqueue.c -- I already implemented these tests, so we might as well enable them. --- changes/workqueue_tests | 4 ++++ src/test/include.am | 18 ++++++++++++++++-- src/test/test_workqueue.c | 3 +++ src/test/test_workqueue_cancel.sh | 4 ++++ src/test/test_workqueue_efd.sh | 4 ++++ src/test/test_workqueue_efd2.sh | 4 ++++ src/test/test_workqueue_pipe.sh | 4 ++++ src/test/test_workqueue_pipe2.sh | 4 ++++ src/test/test_workqueue_socketpair.sh | 4 ++++ 9 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 changes/workqueue_tests create mode 100755 src/test/test_workqueue_cancel.sh create mode 100755 src/test/test_workqueue_efd.sh create mode 100755 src/test/test_workqueue_efd2.sh create mode 100755 src/test/test_workqueue_pipe.sh create mode 100755 src/test/test_workqueue_pipe2.sh create mode 100755 src/test/test_workqueue_socketpair.sh diff --git a/changes/workqueue_tests b/changes/workqueue_tests new file mode 100644 index 0000000000..7c5d756f76 --- /dev/null +++ b/changes/workqueue_tests @@ -0,0 +1,4 @@ + o Testing: + - Run more workqueue tests as part of "make check". These had previously + been implemented, but you needed to know special command-line options + to enable them. diff --git a/src/test/include.am b/src/test/include.am index fa660f5302..5a91c74cde 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -9,6 +9,12 @@ TESTS_ENVIRONMENT = \ export TESTING_TOR_BINARY="$(TESTING_TOR_BINARY)"; TESTSCRIPTS = src/test/test_zero_length_keys.sh \ + src/test/test_workqueue_cancel.sh \ + src/test/test_workqueue_efd.sh \ + src/test/test_workqueue_efd2.sh \ + src/test/test_workqueue_pipe.sh \ + src/test/test_workqueue_pipe2.sh \ + src/test/test_workqueue_socketpair.sh \ src/test/test_switch_id.sh if USEPYTHON @@ -16,7 +22,8 @@ TESTSCRIPTS += src/test/test_ntor.sh src/test/test_bt.sh endif TESTS += src/test/test src/test/test-slow src/test/test-memwipe \ - src/test/test_workqueue src/test/test_keygen.sh \ + src/test/test_workqueue \ + src/test/test_keygen.sh \ src/test/test-timers \ $(TESTSCRIPTS) @@ -257,4 +264,11 @@ EXTRA_DIST += \ src/test/test_zero_length_keys.sh \ src/test/test_ntor.sh src/test/test_bt.sh \ src/test/test-network.sh \ - src/test/test_switch_id.sh + src/test/test_switch_id.sh \ + src/test/test_workqueue_cancel.sh \ + src/test/test_workqueue_efd.sh \ + src/test/test_workqueue_efd2.sh \ + src/test/test_workqueue_pipe.sh \ + src/test/test_workqueue_pipe2.sh \ + src/test/test_workqueue_socketpair.sh + diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index cbcf596b22..75d68a7ce0 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -400,6 +400,9 @@ main(int argc, char **argv) } rq = replyqueue_new(as_flags); + if (as_flags && rq == NULL) + return 77; // 77 means "skipped". + tor_assert(rq); tp = threadpool_new(opt_n_threads, rq, new_state, free_state, NULL); diff --git a/src/test/test_workqueue_cancel.sh b/src/test/test_workqueue_cancel.sh new file mode 100755 index 0000000000..f7c663171e --- /dev/null +++ b/src/test/test_workqueue_cancel.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue -C 1 + diff --git a/src/test/test_workqueue_efd.sh b/src/test/test_workqueue_efd.sh new file mode 100755 index 0000000000..4d89396819 --- /dev/null +++ b/src/test/test_workqueue_efd.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue \ + --no-eventfd2 --no-pipe2 --no-pipe --no-socketpair diff --git a/src/test/test_workqueue_efd2.sh b/src/test/test_workqueue_efd2.sh new file mode 100755 index 0000000000..7cfff45ff3 --- /dev/null +++ b/src/test/test_workqueue_efd2.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue \ + --no-eventfd --no-pipe2 --no-pipe --no-socketpair diff --git a/src/test/test_workqueue_pipe.sh b/src/test/test_workqueue_pipe.sh new file mode 100755 index 0000000000..afcef87853 --- /dev/null +++ b/src/test/test_workqueue_pipe.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue \ + --no-eventfd2 --no-eventfd --no-pipe2 --no-socketpair diff --git a/src/test/test_workqueue_pipe2.sh b/src/test/test_workqueue_pipe2.sh new file mode 100755 index 0000000000..a20a1427e0 --- /dev/null +++ b/src/test/test_workqueue_pipe2.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue \ + --no-eventfd2 --no-eventfd --no-pipe --no-socketpair diff --git a/src/test/test_workqueue_socketpair.sh b/src/test/test_workqueue_socketpair.sh new file mode 100755 index 0000000000..76af79746d --- /dev/null +++ b/src/test/test_workqueue_socketpair.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +${builddir:-.}/src/test/test_workqueue \ + --no-eventfd2 --no-eventfd --no-pipe2 --no-pipe From 429d15c5291d9d6668457b7070f8c5fc2c6994e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 8 Jun 2016 17:30:22 -0400 Subject: [PATCH 2/2] Mark the unreachable lines in compat_{,p}threads and workqueue These are all related to failures from functions that either can't fail as we call them, or where we cannot provoke failure. --- src/common/compat_pthreads.c | 30 +++++++++++++++++++++--------- src/common/compat_threads.c | 13 +++++++++++-- src/common/workqueue.c | 17 +++++++++++++++-- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 1b24cc3c2a..c414e10a08 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -104,11 +104,13 @@ void tor_mutex_init(tor_mutex_t *mutex) { if (PREDICT_UNLIKELY(!threads_initialized)) - tor_threads_init(); + tor_threads_init(); // LCOV_EXCL_LINE const int err = pthread_mutex_init(&mutex->mutex, &attr_recursive); if (PREDICT_UNLIKELY(err)) { + // LCOV_EXCL_START log_err(LD_GENERAL, "Error %d creating a mutex.", err); - tor_fragile_assert(); + tor_assert_unreached(); + // LCOV_EXCL_STOP } } @@ -118,12 +120,14 @@ void tor_mutex_init_nonrecursive(tor_mutex_t *mutex) { int err; - if (PREDICT_UNLIKELY(!threads_initialized)) - tor_threads_init(); + if (!threads_initialized) + tor_threads_init(); // LCOV_EXCL_LINE err = pthread_mutex_init(&mutex->mutex, NULL); if (PREDICT_UNLIKELY(err)) { + // LCOV_EXCL_START log_err(LD_GENERAL, "Error %d creating a mutex.", err); - tor_fragile_assert(); + tor_assert_unreached(); + // LCOV_EXCL_STOP } } @@ -135,8 +139,10 @@ tor_mutex_acquire(tor_mutex_t *m) tor_assert(m); err = pthread_mutex_lock(&m->mutex); if (PREDICT_UNLIKELY(err)) { + // LCOV_EXCL_START log_err(LD_GENERAL, "Error %d locking a mutex.", err); - tor_fragile_assert(); + tor_assert_unreached(); + // LCOV_EXCL_STOP } } /** Release the lock m so another thread can have it. */ @@ -147,8 +153,10 @@ tor_mutex_release(tor_mutex_t *m) tor_assert(m); err = pthread_mutex_unlock(&m->mutex); if (PREDICT_UNLIKELY(err)) { + // LCOV_EXCL_START log_err(LD_GENERAL, "Error %d unlocking a mutex.", err); - tor_fragile_assert(); + tor_assert_unreached(); + // LCOV_EXCL_STOP } } /** Clean up the mutex m so that it no longer uses any system @@ -161,8 +169,10 @@ tor_mutex_uninit(tor_mutex_t *m) tor_assert(m); err = pthread_mutex_destroy(&m->mutex); if (PREDICT_UNLIKELY(err)) { + // LCOV_EXCL_START log_err(LD_GENERAL, "Error %d destroying a mutex.", err); - tor_fragile_assert(); + tor_assert_unreached(); + // LCOV_EXCL_STOP } } /** Return an integer representing this thread. */ @@ -212,8 +222,10 @@ void tor_cond_uninit(tor_cond_t *cond) { if (pthread_cond_destroy(&cond->cond)) { + // LCOV_EXCL_START log_warn(LD_GENERAL,"Error freeing condition: %s", strerror(errno)); return; + // LCOV_EXCL_STOP } } /** Wait until one of the tor_cond_signal functions is called on cond. @@ -234,7 +246,7 @@ tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) /* EINTR should be impossible according to POSIX, but POSIX, like the * Pirate's Code, is apparently treated "more like what you'd call * guidelines than actual rules." */ - continue; + continue; // LCOV_EXCL_LINE } return r ? -1 : 0; } diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 8f9001258a..e8450186f8 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -63,8 +63,8 @@ tor_cond_t * tor_cond_new(void) { tor_cond_t *cond = tor_malloc(sizeof(tor_cond_t)); - if (tor_cond_init(cond)<0) - tor_free(cond); + if (BUG(tor_cond_init(cond)<0)) + tor_free(cond); // LCOV_EXCL_LINE return cond; } @@ -242,8 +242,11 @@ alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags) if (socks[0] >= 0) { if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || set_socket_nonblocking(socks[0]) < 0) { + // LCOV_EXCL_START -- if eventfd succeeds, fcntl will. + tor_assert_nonfatal_unreached(); close(socks[0]); return -1; + // LCOV_EXCL_STOP } } } @@ -277,9 +280,12 @@ alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags) fcntl(socks[1], F_SETFD, FD_CLOEXEC) < 0 || set_socket_nonblocking(socks[0]) < 0 || set_socket_nonblocking(socks[1]) < 0) { + // LCOV_EXCL_START -- if pipe succeeds, you can fcntl the output + tor_assert_nonfatal_unreached(); close(socks[0]); close(socks[1]); return -1; + // LCOV_EXCL_STOP } socks_out->read_fd = socks[0]; socks_out->write_fd = socks[1]; @@ -294,9 +300,12 @@ alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags) tor_socketpair(AF_UNIX, SOCK_STREAM, 0, socks) == 0) { if (set_socket_nonblocking(socks[0]) < 0 || set_socket_nonblocking(socks[1])) { + // LCOV_EXCL_START -- if socketpair worked, you can make it nonblocking. + tor_assert_nonfatal_unreached(); tor_close_socket(socks[0]); tor_close_socket(socks[1]); return -1; + // LCOV_EXCL_STOP } socks_out->read_fd = socks[0]; socks_out->write_fd = socks[1]; diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 0a38550de0..48c0cca01f 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -262,9 +262,12 @@ workerthread_new(void *state, threadpool_t *pool, replyqueue_t *replyqueue) thr->in_pool = pool; if (spawn_func(worker_thread_main, thr) < 0) { + //LCOV_EXCL_START + tor_assert_nonfatal_unreached(); log_err(LD_GENERAL, "Can't launch worker thread."); tor_free(thr); return NULL; + //LCOV_EXCL_STOP } return thr; @@ -375,8 +378,8 @@ threadpool_queue_update(threadpool_t *pool, static int threadpool_start_threads(threadpool_t *pool, int n) { - if (n < 0) - return -1; + if (BUG(n < 0)) + return -1; // LCOV_EXCL_LINE if (n > MAX_THREADS) n = MAX_THREADS; @@ -391,9 +394,12 @@ threadpool_start_threads(threadpool_t *pool, int n) workerthread_t *thr = workerthread_new(state, pool, pool->reply_queue); if (!thr) { + //LCOV_EXCL_START + tor_assert_nonfatal_unreached(); pool->free_thread_state_fn(state); tor_mutex_release(&pool->lock); return -1; + //LCOV_EXCL_STOP } thr->index = pool->n_threads; pool->threads[pool->n_threads++] = thr; @@ -429,10 +435,13 @@ threadpool_new(int n_threads, pool->reply_queue = replyqueue; if (threadpool_start_threads(pool, n_threads) < 0) { + //LCOV_EXCL_START + tor_assert_nonfatal_unreached(); tor_cond_uninit(&pool->condition); tor_mutex_uninit(&pool->lock); tor_free(pool); return NULL; + //LCOV_EXCL_STOP } return pool; @@ -456,8 +465,10 @@ replyqueue_new(uint32_t alertsocks_flags) rq = tor_malloc_zero(sizeof(replyqueue_t)); if (alert_sockets_create(&rq->alert, alertsocks_flags) < 0) { + //LCOV_EXCL_START tor_free(rq); return NULL; + //LCOV_EXCL_STOP } tor_mutex_init(&rq->lock); @@ -486,10 +497,12 @@ void replyqueue_process(replyqueue_t *queue) { if (queue->alert.drain_fn(queue->alert.read_fd) < 0) { + //LCOV_EXCL_START static ratelim_t warn_limit = RATELIM_INIT(7200); log_fn_ratelim(&warn_limit, LOG_WARN, LD_GENERAL, "Failure from drain_fd: %s", tor_socket_strerror(tor_socket_errno(queue->alert.read_fd))); + //LCOV_EXCL_STOP } tor_mutex_acquire(&queue->lock);