From 4887037cd08e567596339dbb3d65557b8e25560d Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 18 Oct 2012 19:02:04 +0530 Subject: [PATCH 01/16] [2236] Add a --enable-debug configure flag --- configure.ac | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/configure.ac b/configure.ac index 09de67fd2b..fcaa8f319e 100644 --- a/configure.ac +++ b/configure.ac @@ -12,6 +12,18 @@ AC_CONFIG_MACRO_DIR([m4macros]) # Checks for programs. AC_PROG_CXX +# Enable debugging facilities? +AC_ARG_ENABLE([debug], + AS_HELP_STRING([--enable-debug], + [enable debugging (default is no)]), + [case "${enableval}" in + yes) debug_enabled=true ;; + no) debug_enabled=false ;; + *) AC_MSG_ERROR([bad value ${enableval} for --enable-debug]) ;; + esac],[debug_enabled=false]) +AM_CONDITIONAL([DEBUG_ENABLED], [test x$debug = xtrue]) +AM_COND_IF([DEBUG_ENABLED], [AC_DEFINE([ENABLE_DEBUG], [1], [Enable debugging facilities?])]) + # Libtool configuration # From 2a6f0764001c25acb40eae27d7a2f7691d4c767b Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 18 Oct 2012 19:02:36 +0530 Subject: [PATCH 02/16] [2236] Use ENABLE_DEBUG within the lock implementation --- src/lib/util/threads/sync.cc | 66 ++++++++++++------- src/lib/util/threads/sync.h | 4 ++ .../util/threads/tests/condvar_unittest.cc | 4 ++ src/lib/util/threads/tests/lock_unittest.cc | 8 +++ 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/lib/util/threads/sync.cc b/src/lib/util/threads/sync.cc index c98a7a6b06..9c15f0378c 100644 --- a/src/lib/util/threads/sync.cc +++ b/src/lib/util/threads/sync.cc @@ -31,12 +31,16 @@ namespace thread { class Mutex::Impl { public: - Impl() : - locked_count(0) + Impl() +#ifdef ENABLE_DEBUG + : locked_count(0) +#endif // ENABLE_DEBUG {} + pthread_mutex_t mutex; - // Only in debug mode +#ifdef ENABLE_DEBUG size_t locked_count; +#endif // ENABLE_DEBUG }; namespace { @@ -93,12 +97,17 @@ Mutex::Mutex() : Mutex::~Mutex() { if (impl_ != NULL) { const int result = pthread_mutex_destroy(&impl_->mutex); + +#ifdef ENABLE_DEBUG const bool locked = impl_->locked_count != 0; +#endif // ENABLE_DEBUG + delete impl_; // We don't want to throw from the destructor. Also, if this ever // fails, something is really screwed up a lot. assert(result == 0); +#ifdef ENABLE_DEBUG // We should not try to destroy a locked mutex, bad threaded monsters // could get loose if we ever do and it is also forbidden by pthreads. @@ -106,9 +115,12 @@ Mutex::~Mutex() { // pthread_mutex_destroy should check for it already. But it seems // there are systems that don't check it. assert(!locked); +#endif // ENABLE_DEBUG } } +#ifdef ENABLE_DEBUG + void Mutex::postLockAction() { // This assertion would fail only in non-debugging mode, in which case @@ -118,16 +130,6 @@ Mutex::postLockAction() { ++impl_->locked_count; } -void -Mutex::lock() { - assert(impl_ != NULL); - const int result = pthread_mutex_lock(&impl_->mutex); - if (result != 0) { - isc_throw(isc::InvalidOperation, std::strerror(result)); - } - postLockAction(); // Only in debug mode -} - void Mutex::preUnlockAction(bool throw_ok) { if (impl_->locked_count == 0) { @@ -141,20 +143,35 @@ Mutex::preUnlockAction(bool throw_ok) { --impl_->locked_count; } -void -Mutex::unlock() { - assert(impl_ != NULL); - preUnlockAction(false); // Only in debug mode. Ensure no throw. - const int result = pthread_mutex_unlock(&impl_->mutex); - assert(result == 0); // This should never be possible -} - -// TODO: Disable in non-debug build bool Mutex::locked() const { return (impl_->locked_count != 0); } +#endif // ENABLE_DEBUG + +void +Mutex::lock() { + assert(impl_ != NULL); + const int result = pthread_mutex_lock(&impl_->mutex); + if (result != 0) { + isc_throw(isc::InvalidOperation, std::strerror(result)); + } +#ifdef ENABLE_DEBUG + postLockAction(); // Only in debug mode +#endif // ENABLE_DEBUG +} + +void +Mutex::unlock() { + assert(impl_ != NULL); +#ifdef ENABLE_DEBUG + preUnlockAction(false); // Only in debug mode. Ensure no throw. +#endif // ENABLE_DEBUG + const int result = pthread_mutex_unlock(&impl_->mutex); + assert(result == 0); // This should never be possible +} + class CondVar::Impl { public: Impl() { @@ -187,10 +204,13 @@ CondVar::~CondVar() { void CondVar::wait(Mutex& mutex) { +#ifdef ENABLE_DEBUG mutex.preUnlockAction(true); // Only in debug mode const int result = pthread_cond_wait(&impl_->cond_, &mutex.impl_->mutex); mutex.postLockAction(); // Only in debug mode - +#else + const int result = pthread_cond_wait(&impl_->cond_, &mutex.impl_->mutex); +#endif // pthread_cond_wait should normally succeed unless mutex is completely // broken. if (result != 0) { diff --git a/src/lib/util/threads/sync.h b/src/lib/util/threads/sync.h index ff56999f99..9bc7463af7 100644 --- a/src/lib/util/threads/sync.h +++ b/src/lib/util/threads/sync.h @@ -110,6 +110,8 @@ public: private: friend class CondVar; +#ifdef ENABLE_DEBUG + // Commonly called after acquiring the lock, checking and updating // internal state for debug. void postLockAction(); @@ -123,6 +125,8 @@ private: // when called from a destructor). void preUnlockAction(bool throw_ok); +#endif // ENABLE_DEBUG + class Impl; Impl* impl_; void lock(); diff --git a/src/lib/util/threads/tests/condvar_unittest.cc b/src/lib/util/threads/tests/condvar_unittest.cc index 77a8980704..6cec0dc9b8 100644 --- a/src/lib/util/threads/tests/condvar_unittest.cc +++ b/src/lib/util/threads/tests/condvar_unittest.cc @@ -151,7 +151,11 @@ TEST_F(CondVarTest, DISABLED_destroyWhileWait) { TEST_F(CondVarTest, badWait) { // In our implementation, wait() requires acquiring the lock beforehand. +#ifdef ENABLE_DEBUG EXPECT_THROW(condvar_.wait(mutex_), isc::InvalidOperation); +#else + EXPECT_THROW(condvar_.wait(mutex_), isc::BadValue); +#endif // ENABLE_DEBUG } TEST_F(CondVarTest, emptySignal) { diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc index 9c17e6fc65..abd0f607d8 100644 --- a/src/lib/util/threads/tests/lock_unittest.cc +++ b/src/lib/util/threads/tests/lock_unittest.cc @@ -31,13 +31,21 @@ TEST(MutexTest, lockMultiple) { // TODO: Once we support non-debug mutexes, disable the test if we compile // with them. Mutex mutex; +#ifdef ENABLE_DEBUG EXPECT_FALSE(mutex.locked()); // Debug-only build +#endif // ENABLE_DEBUG + Mutex::Locker l1(mutex); +#ifdef ENABLE_DEBUG EXPECT_TRUE(mutex.locked()); // Debug-only build +#endif // ENABLE_DEBUG + EXPECT_THROW({ Mutex::Locker l2(mutex); // Attempt to lock again. }, isc::InvalidOperation); +#ifdef ENABLE_DEBUG EXPECT_TRUE(mutex.locked()); // Debug-only build +#endif // ENABLE_DEBUG } // Destroying a locked mutex is a bad idea as well From 149df5a690bc874b8814669c19a8666e2cddf961 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 18 Oct 2012 19:12:37 +0530 Subject: [PATCH 03/16] [2236] Add status whether debugging is enabled in configure output This also fixes a bug where the AM_CONDITIONAL was not set properly. --- configure.ac | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index fcaa8f319e..83b9c029fc 100644 --- a/configure.ac +++ b/configure.ac @@ -17,11 +17,11 @@ AC_ARG_ENABLE([debug], AS_HELP_STRING([--enable-debug], [enable debugging (default is no)]), [case "${enableval}" in - yes) debug_enabled=true ;; - no) debug_enabled=false ;; + yes) debug_enabled=yes ;; + no) debug_enabled=no ;; *) AC_MSG_ERROR([bad value ${enableval} for --enable-debug]) ;; - esac],[debug_enabled=false]) -AM_CONDITIONAL([DEBUG_ENABLED], [test x$debug = xtrue]) + esac],[debug_enabled=no]) +AM_CONDITIONAL([DEBUG_ENABLED], [test x$debug_enabled = xyes]) AM_COND_IF([DEBUG_ENABLED], [AC_DEFINE([ENABLE_DEBUG], [1], [Enable debugging facilities?])]) # Libtool configuration @@ -1433,6 +1433,7 @@ Features: $enable_features Developer: + Enable Debugging: $debug_enabled Google Tests: $enable_gtest Valgrind: $found_valgrind C++ Code Coverage: $USE_LCOV From fa78c4ba3aff851955576ec7e98f8fe98dacd889 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 18 Oct 2012 19:13:02 +0530 Subject: [PATCH 04/16] [2236] Include and use the define from config.h --- src/lib/util/threads/sync.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/util/threads/sync.h b/src/lib/util/threads/sync.h index 9bc7463af7..6a968df69d 100644 --- a/src/lib/util/threads/sync.h +++ b/src/lib/util/threads/sync.h @@ -15,6 +15,8 @@ #ifndef B10_THREAD_SYNC_H #define B10_THREAD_SYNC_H +#include "config.h" + #include #include // for NULL. From 7c40eaf7779f7fb3b80ad141c58f60e69a2f3e4e Mon Sep 17 00:00:00 2001 From: "Jeremy C. Reed" Date: Thu, 18 Oct 2012 09:40:55 -0500 Subject: [PATCH 05/16] [trac2361] add new test for CHAOS version.bind and authors.bind TXT --- .../auth/auth_basic.config.orig | 22 +++++++++++++++++++ tests/lettuce/features/auth_basic.feature | 22 +++++++++++++++++++ tests/lettuce/features/terrain/terrain.py | 2 ++ 3 files changed, 46 insertions(+) create mode 100644 tests/lettuce/configurations/auth/auth_basic.config.orig create mode 100644 tests/lettuce/features/auth_basic.feature diff --git a/tests/lettuce/configurations/auth/auth_basic.config.orig b/tests/lettuce/configurations/auth/auth_basic.config.orig new file mode 100644 index 0000000000..4067fb1b27 --- /dev/null +++ b/tests/lettuce/configurations/auth/auth_basic.config.orig @@ -0,0 +1,22 @@ +{ + "version": 2, + "Logging": { + "loggers": [ { + "debuglevel": 99, + "severity": "DEBUG", + "name": "*" + } ] + }, + "Auth": { + "listen_on": [ { + "port": 47806, + "address": "127.0.0.1" + } ] + }, + "Boss": { + "components": { + "b10-auth": { "kind": "needed", "special": "auth" }, + "b10-cmdctl": { "special": "cmdctl", "kind": "needed" } + } + } +} diff --git a/tests/lettuce/features/auth_basic.feature b/tests/lettuce/features/auth_basic.feature new file mode 100644 index 0000000000..3b66f295b6 --- /dev/null +++ b/tests/lettuce/features/auth_basic.feature @@ -0,0 +1,22 @@ +Feature: Basic Authoritative DNS server + This feature set is for testing the execution of the b10-auth + component using its default datasource configurations. This + will start it and perform queries against it. + + Scenario: Query builtin bind zone + Given I have bind10 running with configuration auth/auth_basic.config + And wait for bind10 stderr message BIND10_STARTED_CC + And wait for bind10 stderr message CMDCTL_STARTED + And wait for bind10 stderr message AUTH_SERVER_STARTED + + bind10 module Auth should be running + And bind10 module Resolver should not be running + + A query for example.com should have rcode REFUSED + + A query for version.bind type TXT class CH should have rcode NOERROR + + A query for authors.bind type TXT class CH should have rcode NOERROR + +# TODO: to be compatible with BIND 9 +# A query for nonexistent.bind type TXT class CH should have rcode REFUSED diff --git a/tests/lettuce/features/terrain/terrain.py b/tests/lettuce/features/terrain/terrain.py index 8720e2d994..2bfe7d4f14 100644 --- a/tests/lettuce/features/terrain/terrain.py +++ b/tests/lettuce/features/terrain/terrain.py @@ -49,6 +49,8 @@ copylist = [ "configurations/example.org.config"], ["configurations/bindctl/bindctl.config.orig", "configurations/bindctl/bindctl.config"], + ["configurations/auth/auth_basic.config.orig", + "configurations/auth/auth_basic.config"], ["configurations/resolver/resolver_basic.config.orig", "configurations/resolver/resolver_basic.config"], ["configurations/multi_instance/multi_auth.config.orig", From 3ac353a5a561e5a0ecae750bd0d52afb2fa86037 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Fri, 19 Oct 2012 06:08:14 +0530 Subject: [PATCH 06/16] [2361] Make minor whitespace updates --- tests/lettuce/features/auth_basic.feature | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/lettuce/features/auth_basic.feature b/tests/lettuce/features/auth_basic.feature index 3b66f295b6..4667b6441b 100644 --- a/tests/lettuce/features/auth_basic.feature +++ b/tests/lettuce/features/auth_basic.feature @@ -13,10 +13,8 @@ Feature: Basic Authoritative DNS server And bind10 module Resolver should not be running A query for example.com should have rcode REFUSED - A query for version.bind type TXT class CH should have rcode NOERROR - A query for authors.bind type TXT class CH should have rcode NOERROR -# TODO: to be compatible with BIND 9 -# A query for nonexistent.bind type TXT class CH should have rcode REFUSED + # TODO: to be compatible with BIND 9 + # A query for nonexistent.bind type TXT class CH should have rcode REFUSED From 7161d9d899723f1be561125087e1daaf820e80ed Mon Sep 17 00:00:00 2001 From: "Jeremy C. Reed" Date: Fri, 19 Oct 2012 10:14:44 -0500 Subject: [PATCH 07/16] [trac2361] fix issue where the bind10 could not run in source tree when the installation was not done yet. I added extra local_plugins path and custom datasrc.spec plugin for that. --- configure.ac | 1 + src/bin/cfgmgr/Makefile.am | 2 +- src/bin/cfgmgr/local_plugins/Makefile.am | 10 ++++++++++ src/bin/cfgmgr/plugins/Makefile.am | 2 +- src/bin/cfgmgr/plugins/datasrc.spec.pre.in | 4 ++-- src/lib/python/bind10_config.py.in | 2 ++ tests/lettuce/configurations/auth/.gitignore | 1 + 7 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 src/bin/cfgmgr/local_plugins/Makefile.am create mode 100644 tests/lettuce/configurations/auth/.gitignore diff --git a/configure.ac b/configure.ac index 09de67fd2b..32f81b320f 100644 --- a/configure.ac +++ b/configure.ac @@ -1112,6 +1112,7 @@ AC_CONFIG_FILES([Makefile src/bin/bindctl/Makefile src/bin/bindctl/tests/Makefile src/bin/cfgmgr/Makefile + src/bin/cfgmgr/local_plugins/Makefile src/bin/cfgmgr/plugins/Makefile src/bin/cfgmgr/plugins/tests/Makefile src/bin/cfgmgr/tests/Makefile diff --git a/src/bin/cfgmgr/Makefile.am b/src/bin/cfgmgr/Makefile.am index e9e0ccaf85..9c73f79ee6 100644 --- a/src/bin/cfgmgr/Makefile.am +++ b/src/bin/cfgmgr/Makefile.am @@ -1,4 +1,4 @@ -SUBDIRS = . plugins tests +SUBDIRS = . plugins local_plugins tests pkglibexecdir = $(libexecdir)/@PACKAGE@ diff --git a/src/bin/cfgmgr/local_plugins/Makefile.am b/src/bin/cfgmgr/local_plugins/Makefile.am new file mode 100644 index 0000000000..63e152d790 --- /dev/null +++ b/src/bin/cfgmgr/local_plugins/Makefile.am @@ -0,0 +1,10 @@ +# Nothing is installed from this directory. +# This local_plugins directory is to override plugins when used from running +# within source tree for testing (instead of installation prefix). + +noinst_DATA = datasrc.spec + +datasrc.spec: ../plugins/datasrc.spec.pre + $(SED) -e "s|@@STATIC_ZONE_FILE@@|$(abs_top_builddir)/src/lib/datasrc/static.zone|;s|@@SQLITE3_DATABASE_FILE@@|$(abs_top_builddir)/local.zone.sqlite3|" ../plugins/datasrc.spec.pre >$@ + +CLEANFILES = datasrc.spec diff --git a/src/bin/cfgmgr/plugins/Makefile.am b/src/bin/cfgmgr/plugins/Makefile.am index e6ed1275c8..5967abd4ac 100644 --- a/src/bin/cfgmgr/plugins/Makefile.am +++ b/src/bin/cfgmgr/plugins/Makefile.am @@ -3,7 +3,7 @@ SUBDIRS = tests EXTRA_DIST = README logging.spec tsig_keys.spec datasrc.spec: datasrc.spec.pre - $(SED) -e "s|@@PKGDATADIR@@|$(pkgdatadir)|;s|@@LOCALSTATEDIR@@|$(localstatedir)|" datasrc.spec.pre >$@ + $(SED) -e "s|@@STATIC_ZONE_FILE@@|$(pkgdatadir)/static.zone|;s|@@SQLITE3_DATABASE_FILE@@|$(localstatedir)/$(PACKAGE)/zone.sqlite3|" datasrc.spec.pre >$@ config_plugindir = @prefix@/share/@PACKAGE@/config_plugins config_plugin_DATA = logging.spec tsig_keys.spec datasrc.spec diff --git a/src/bin/cfgmgr/plugins/datasrc.spec.pre.in b/src/bin/cfgmgr/plugins/datasrc.spec.pre.in index f2c6a974a2..6d5bd77e75 100644 --- a/src/bin/cfgmgr/plugins/datasrc.spec.pre.in +++ b/src/bin/cfgmgr/plugins/datasrc.spec.pre.in @@ -12,7 +12,7 @@ { "type": "sqlite3", "params": { - "database_file": "@@LOCALSTATEDIR@@/@PACKAGE@/zone.sqlite3" + "database_file": "@@SQLITE3_DATABASE_FILE@@" } } ], @@ -20,7 +20,7 @@ { "type": "static", "cache-enable": false, - "params": "@@PKGDATADIR@@/static.zone" + "params": "@@STATIC_ZONE_FILE@@" } ] }, diff --git a/src/lib/python/bind10_config.py.in b/src/lib/python/bind10_config.py.in index b8975cf625..c765fa29a1 100644 --- a/src/lib/python/bind10_config.py.in +++ b/src/lib/python/bind10_config.py.in @@ -60,6 +60,8 @@ def reload(): else: DATA_PATH = os.environ["B10_FROM_SOURCE"] PLUGIN_PATHS = [os.environ["B10_FROM_SOURCE"] + + '/src/bin/cfgmgr/local_plugins', + os.environ["B10_FROM_SOURCE"] + '/src/bin/cfgmgr/plugins'] programdirs = ['auth', 'cfgmgr', 'cmdctl', 'ddns', 'dhcp6', 'msgq', 'resolver', 'sockcreator', 'stats', 'xfrin', 'xfrout', diff --git a/tests/lettuce/configurations/auth/.gitignore b/tests/lettuce/configurations/auth/.gitignore new file mode 100644 index 0000000000..07f1b7db04 --- /dev/null +++ b/tests/lettuce/configurations/auth/.gitignore @@ -0,0 +1 @@ +/auth_basic.config From c62976b616bd2b44a9f791c47afde5d89ef9913a Mon Sep 17 00:00:00 2001 From: "Jeremy C. Reed" Date: Fri, 19 Oct 2012 10:17:23 -0500 Subject: [PATCH 08/16] [trac2361] typo fix in comment noticed this while editing file for previous commit --- src/lib/python/bind10_config.py.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/python/bind10_config.py.in b/src/lib/python/bind10_config.py.in index c765fa29a1..6db64e26c0 100644 --- a/src/lib/python/bind10_config.py.in +++ b/src/lib/python/bind10_config.py.in @@ -51,7 +51,7 @@ def reload(): # tree the programs in the tree (not installed ones) will be used. # # B10_FROM_SOURCE_LOCALSTATEDIR is specifically intended to be used for - # tests where we want to use variuos types of configuration within the test + # tests where we want to use various types of configuration within the test # environment. (We may want to make it even more generic so that the path # is passed from the boss process) if "B10_FROM_SOURCE" in os.environ: From 77e32834470282fe08ae7fb8c8fb9a4afb163677 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 07:36:52 +0530 Subject: [PATCH 09/16] [2361] Update comment about local_plugins directory --- src/bin/cfgmgr/local_plugins/Makefile.am | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bin/cfgmgr/local_plugins/Makefile.am b/src/bin/cfgmgr/local_plugins/Makefile.am index 63e152d790..2f4dd39767 100644 --- a/src/bin/cfgmgr/local_plugins/Makefile.am +++ b/src/bin/cfgmgr/local_plugins/Makefile.am @@ -1,6 +1,7 @@ -# Nothing is installed from this directory. -# This local_plugins directory is to override plugins when used from running -# within source tree for testing (instead of installation prefix). +# Nothing is installed from this directory. This local_plugins +# directory overrides the plugins directory when lettuce is run, and the +# spec file here is used to serve the static zone from the source tree +# for testing (instead of installation prefix). noinst_DATA = datasrc.spec From 4f4b3724e1e06933646dcbee14f31eb2713a514d Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 09:24:50 +0530 Subject: [PATCH 10/16] [2236] Access Mutex::locked() only when ENABLE_DEBUG is defined --- src/bin/auth/auth_srv.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc index 315a7527de..bf653a486e 100644 --- a/src/bin/auth/auth_srv.cc +++ b/src/bin/auth/auth_srv.cc @@ -274,10 +274,12 @@ public: shared_ptr getDataSrcClientList( const RRClass& rrclass) { - // TODO: Debug-build only check +#ifdef ENABLE_DEBUG + // Debug-build only check if (!mutex_.locked()) { isc_throw(isc::Unexpected, "Not locked!"); } +#endif const std::map >:: const_iterator it(datasrc_client_lists_->find(rrclass)); if (it == datasrc_client_lists_->end()) { @@ -935,10 +937,12 @@ AuthSrv::destroyDDNSForwarder() { AuthSrv::DataSrcClientListsPtr AuthSrv::swapDataSrcClientLists(DataSrcClientListsPtr new_lists) { - // TODO: Debug-build only check +#ifdef ENABLE_DEBUG + // Debug-build only check if (!impl_->mutex_.locked()) { isc_throw(isc::Unexpected, "Not locked!"); } +#endif std::swap(new_lists, impl_->datasrc_client_lists_); return (new_lists); } From 98db15a31fea52ec2b1952846c61d20f9f373305 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 09:25:20 +0530 Subject: [PATCH 11/16] [2236] Add comments to configure.ac about --enable-debug and performance --- configure.ac | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 83b9c029fc..922631bb70 100644 --- a/configure.ac +++ b/configure.ac @@ -12,7 +12,9 @@ AC_CONFIG_MACRO_DIR([m4macros]) # Checks for programs. AC_PROG_CXX -# Enable debugging facilities? +# Enable low-performing debugging facilities? This option optionally +# enables some debugging aids that perform slowly and hence aren't built +# by default. AC_ARG_ENABLE([debug], AS_HELP_STRING([--enable-debug], [enable debugging (default is no)]), @@ -22,7 +24,7 @@ AC_ARG_ENABLE([debug], *) AC_MSG_ERROR([bad value ${enableval} for --enable-debug]) ;; esac],[debug_enabled=no]) AM_CONDITIONAL([DEBUG_ENABLED], [test x$debug_enabled = xyes]) -AM_COND_IF([DEBUG_ENABLED], [AC_DEFINE([ENABLE_DEBUG], [1], [Enable debugging facilities?])]) +AM_COND_IF([DEBUG_ENABLED], [AC_DEFINE([ENABLE_DEBUG], [1], [Enable low-performing debugging facilities?])]) # Libtool configuration # From 8b19f3a80a57b4a0c41e58460920924d5613e606 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 09:28:43 +0530 Subject: [PATCH 12/16] [2236] Remove config.h include from sync.h ... and move it to sync.cc. This is so that sync.h can be dist-ed. Also add comments to some methods that may not be available in non-debug builds. --- src/lib/util/threads/sync.cc | 2 ++ src/lib/util/threads/sync.h | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/lib/util/threads/sync.cc b/src/lib/util/threads/sync.cc index 9c15f0378c..c2f4fed48c 100644 --- a/src/lib/util/threads/sync.cc +++ b/src/lib/util/threads/sync.cc @@ -12,6 +12,8 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. +#include "config.h" + #include "sync.h" #include diff --git a/src/lib/util/threads/sync.h b/src/lib/util/threads/sync.h index 6a968df69d..87c78be94d 100644 --- a/src/lib/util/threads/sync.h +++ b/src/lib/util/threads/sync.h @@ -15,8 +15,6 @@ #ifndef B10_THREAD_SYNC_H #define B10_THREAD_SYNC_H -#include "config.h" - #include #include // for NULL. @@ -112,10 +110,11 @@ public: private: friend class CondVar; -#ifdef ENABLE_DEBUG - // Commonly called after acquiring the lock, checking and updating // internal state for debug. + // + // Note that this method is only available when the build is + // configured with debugging support. void postLockAction(); // Commonly called before releasing the lock, checking and updating @@ -125,10 +124,11 @@ private: // fails; otherwise it aborts the process. This parameter must be set // to false if the call to this shouldn't result in an exception (e.g. // when called from a destructor). + // + // Note that this method is only available when the build is + // configured with debugging support. void preUnlockAction(bool throw_ok); -#endif // ENABLE_DEBUG - class Impl; Impl* impl_; void lock(); From 2eaf01995b4c0d6c007afd51c94b8453cbb797ca Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 09:36:26 +0530 Subject: [PATCH 13/16] [2236] Don't use PTHREAD_MUTEX_ERRORCHECK in non-debug builds Also disable tests in non-debug builds that depend on PTHREAD_MUTEX_ERRORCHECK. --- src/bin/auth/tests/auth_srv_unittest.cc | 4 ++++ src/lib/util/threads/sync.cc | 5 ++++- src/lib/util/threads/tests/condvar_unittest.cc | 8 ++++---- src/lib/util/threads/tests/lock_unittest.cc | 14 +++++++------- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index 396b247491..cde4c8169b 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -1824,6 +1824,8 @@ TEST_F(AuthSrvTest, clientList) { EXPECT_FALSE(server.getDataSrcClientList(RRClass::CH())); } +#ifdef ENABLE_DEBUG + // We just test the mutex can be locked (exactly once). TEST_F(AuthSrvTest, mutex) { isc::util::thread::Mutex::Locker l1(server.getDataSrcClientListMutex()); @@ -1837,4 +1839,6 @@ TEST_F(AuthSrvTest, mutex) { }, isc::InvalidOperation); } +#endif // ENABLE_DEBUG + } diff --git a/src/lib/util/threads/sync.cc b/src/lib/util/threads/sync.cc index c2f4fed48c..ca467b63d4 100644 --- a/src/lib/util/threads/sync.cc +++ b/src/lib/util/threads/sync.cc @@ -76,12 +76,15 @@ Mutex::Mutex() : isc_throw(isc::InvalidOperation, std::strerror(result)); } Deinitializer deinitializer(attributes); + +#ifdef ENABLE_DEBUG // TODO: Distinguish if debug mode is enabled in compilation. - // If so, it should be PTHREAD_MUTEX_NORMAL or NULL result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_ERRORCHECK); if (result != 0) { isc_throw(isc::InvalidOperation, std::strerror(result)); } +#endif // ENABLE_DEBUG + auto_ptr impl(new Impl); result = pthread_mutex_init(&impl->mutex, &attributes); switch (result) { diff --git a/src/lib/util/threads/tests/condvar_unittest.cc b/src/lib/util/threads/tests/condvar_unittest.cc index 6cec0dc9b8..8b1fe5445e 100644 --- a/src/lib/util/threads/tests/condvar_unittest.cc +++ b/src/lib/util/threads/tests/condvar_unittest.cc @@ -149,15 +149,15 @@ TEST_F(CondVarTest, DISABLED_destroyWhileWait) { }, ""); } +#ifdef ENABLE_DEBUG + TEST_F(CondVarTest, badWait) { // In our implementation, wait() requires acquiring the lock beforehand. -#ifdef ENABLE_DEBUG EXPECT_THROW(condvar_.wait(mutex_), isc::InvalidOperation); -#else - EXPECT_THROW(condvar_.wait(mutex_), isc::BadValue); -#endif // ENABLE_DEBUG } +#endif // ENABLE_DEBUG + TEST_F(CondVarTest, emptySignal) { // It's okay to call signal when no one waits. EXPECT_NO_THROW(condvar_.signal()); diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc index abd0f607d8..40af7eee6c 100644 --- a/src/lib/util/threads/tests/lock_unittest.cc +++ b/src/lib/util/threads/tests/lock_unittest.cc @@ -26,28 +26,28 @@ using namespace isc::util::thread; namespace { -// If we try to lock the debug mutex multiple times, it should throw. +#ifdef ENABLE_DEBUG + +// If we try to lock the debug mutex multiple times, it should +// throw. This test will complete properly only when pthread debugging +// facilities are enabled by configuring the code for debug build. TEST(MutexTest, lockMultiple) { // TODO: Once we support non-debug mutexes, disable the test if we compile // with them. Mutex mutex; -#ifdef ENABLE_DEBUG EXPECT_FALSE(mutex.locked()); // Debug-only build -#endif // ENABLE_DEBUG Mutex::Locker l1(mutex); -#ifdef ENABLE_DEBUG EXPECT_TRUE(mutex.locked()); // Debug-only build -#endif // ENABLE_DEBUG EXPECT_THROW({ Mutex::Locker l2(mutex); // Attempt to lock again. }, isc::InvalidOperation); -#ifdef ENABLE_DEBUG EXPECT_TRUE(mutex.locked()); // Debug-only build -#endif // ENABLE_DEBUG } +#endif // ENABLE_DEBUG + // Destroying a locked mutex is a bad idea as well #ifdef EXPECT_DEATH TEST(MutexTest, destroyLocked) { From 7c4e02c157bf7a4baaa81c448ff485e7dec31182 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Tue, 23 Oct 2012 10:25:14 +0530 Subject: [PATCH 14/16] [2236] Remove obsolete comment --- src/lib/util/threads/sync.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/lib/util/threads/sync.cc b/src/lib/util/threads/sync.cc index ca467b63d4..cf68c5e912 100644 --- a/src/lib/util/threads/sync.cc +++ b/src/lib/util/threads/sync.cc @@ -128,9 +128,6 @@ Mutex::~Mutex() { void Mutex::postLockAction() { - // This assertion would fail only in non-debugging mode, in which case - // this method wouldn't be called either, so we simply assert the - // condition. assert(impl_->locked_count == 0); ++impl_->locked_count; } From 1d4d9e796c10786417099a69e8c4e6e4c1cdfb8b Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 25 Oct 2012 06:49:58 +0530 Subject: [PATCH 15/16] [2236] Use PTHREAD_MUTEX_NORMAL fast mutexes by default --- src/lib/util/threads/sync.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lib/util/threads/sync.cc b/src/lib/util/threads/sync.cc index cf68c5e912..46a56465a4 100644 --- a/src/lib/util/threads/sync.cc +++ b/src/lib/util/threads/sync.cc @@ -77,13 +77,18 @@ Mutex::Mutex() : } Deinitializer deinitializer(attributes); + // If debug mode is enabled in compilation, use the slower + // error-checking mutexes that detect deadlocks. Otherwise, use fast + // mutexes which don't. See the pthread_mutexattr_settype() POSIX + // documentation which describes these type attributes. #ifdef ENABLE_DEBUG - // TODO: Distinguish if debug mode is enabled in compilation. result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_ERRORCHECK); +#else + result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_NORMAL); +#endif // ENABLE_DEBUG if (result != 0) { isc_throw(isc::InvalidOperation, std::strerror(result)); } -#endif // ENABLE_DEBUG auto_ptr impl(new Impl); result = pthread_mutex_init(&impl->mutex, &attributes); From 54ae641d1c1fa87baeb702ec0ee25b28c2b7aebd Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 25 Oct 2012 07:04:08 +0530 Subject: [PATCH 16/16] [2236] Remove obsolete comment --- src/lib/util/threads/tests/lock_unittest.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc index 40af7eee6c..498d01e00a 100644 --- a/src/lib/util/threads/tests/lock_unittest.cc +++ b/src/lib/util/threads/tests/lock_unittest.cc @@ -32,8 +32,6 @@ namespace { // throw. This test will complete properly only when pthread debugging // facilities are enabled by configuring the code for debug build. TEST(MutexTest, lockMultiple) { - // TODO: Once we support non-debug mutexes, disable the test if we compile - // with them. Mutex mutex; EXPECT_FALSE(mutex.locked()); // Debug-only build