From: Mukund Sivaraman <muks@isc.org>
Date: Fri, 1 Jun 2012 06:47:59 +0000 (+0530)
Subject: [1704] Use the is_locked_ flag to call unlock() just once
X-Git-Tag: trac2351_base~226^2~60^2^2~9
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7987b09972865cf558dbff4489a198162ec8e3c3;p=thirdparty%2Fkea.git

[1704] Use the is_locked_ flag to call unlock() just once
---

diff --git a/src/lib/log/tests/logger_lock_test.sh.in b/src/lib/log/tests/logger_lock_test.sh.in
index a1f586598c..0324499329 100755
--- a/src/lib/log/tests/logger_lock_test.sh.in
+++ b/src/lib/log/tests/logger_lock_test.sh.in
@@ -34,7 +34,6 @@ cat > $tempfile << .
 LOGGER_LOCK_TEST: LOCK
 INFO  [bind10.log] LOG_LOCK_TEST_MESSAGE this is a test message.
 LOGGER_LOCK_TEST: UNLOCK
-LOGGER_LOCK_TEST: UNLOCK
 .
 rm -f $destfile
 B10_LOGGER_SEVERITY=INFO B10_LOGGER_DESTINATION=stdout ./logger_lock_test > $destfile
diff --git a/src/lib/util/interprocess_sync.h b/src/lib/util/interprocess_sync.h
index b04c37fb13..bcf46c0655 100644
--- a/src/lib/util/interprocess_sync.h
+++ b/src/lib/util/interprocess_sync.h
@@ -38,6 +38,10 @@ class InterprocessSyncLocker; // forward declaration
 /// 4. Client performs task that needs mutual exclusion.
 /// 5. Client frees lock with unlock(), or simply returns from the basic
 /// block which forms the scope for the InterprocessSyncLocker.
+///
+/// NOTE: All implementations of InterprocessSync should keep the
+/// is_locked_ member variable updated whenever their
+/// lock()/tryLock()/unlock() implementations are called.
 class InterprocessSync {
   // InterprocessSyncLocker is the only code outside this class that
   // should be allowed to call the lock(), tryLock() and unlock()
@@ -100,7 +104,8 @@ public:
 
     /// \brief Destructor
     ~InterprocessSyncLocker() {
-        unlock();
+        if (isLocked())
+            unlock();
     }
 
     /// \brief Acquire the lock (blocks if something else has acquired a
@@ -113,11 +118,20 @@ public:
 
     /// \brief Try to acquire a lock (doesn't block)
     ///
-    /// \return Returns true if the lock was acquired, false otherwise.
+    /// \return Returns true if a new lock could be acquired, false
+    ///         otherwise.
     bool tryLock() {
         return (sync_.tryLock());
     }
 
+    /// \brief Check if the lock is taken
+    ///
+    /// \return Returns true if a lock is currently acquired, false
+    ///         otherwise.
+    bool isLocked() {
+        return (sync_.is_locked_);
+    }
+
     /// \brief Release the lock
     ///
     /// \return Returns true if the lock was released, false otherwise.
diff --git a/src/lib/util/interprocess_sync_null.cc b/src/lib/util/interprocess_sync_null.cc
index e2cfc9b95a..5355d5727e 100644
--- a/src/lib/util/interprocess_sync_null.cc
+++ b/src/lib/util/interprocess_sync_null.cc
@@ -22,16 +22,19 @@ InterprocessSyncNull::~InterprocessSyncNull() {
 
 bool
 InterprocessSyncNull::lock() {
+    is_locked_ = true;
     return (true);
 }
 
 bool
 InterprocessSyncNull::tryLock() {
+    is_locked_ = true;
     return (true);
 }
 
 bool
 InterprocessSyncNull::unlock() {
+    is_locked_ = false;
     return (true);
 }
 
diff --git a/src/lib/util/tests/interprocess_sync_file_unittest.cc b/src/lib/util/tests/interprocess_sync_file_unittest.cc
index 46f96cc6aa..a0b4109203 100644
--- a/src/lib/util/tests/interprocess_sync_file_unittest.cc
+++ b/src/lib/util/tests/interprocess_sync_file_unittest.cc
@@ -55,7 +55,9 @@ TEST(InterprocessSyncFileTest, TestLock) {
   InterprocessSyncFile sync("test");
   InterprocessSyncLocker locker(sync);
 
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.lock());
+  EXPECT_TRUE(locker.isLocked());
 
   int fds[2];
 
@@ -77,7 +79,10 @@ TEST(InterprocessSyncFileTest, TestLock) {
       InterprocessSyncLocker locker2(sync2);
 
       if (!locker2.tryLock()) {
+          EXPECT_FALSE(locker2.isLocked());
           locked = 1;
+      } else {
+          EXPECT_TRUE(locker2.isLocked());
       }
 
       write(fds[1], &locked, sizeof(locked));
@@ -95,6 +100,7 @@ TEST(InterprocessSyncFileTest, TestLock) {
   }
 
   EXPECT_TRUE(locker.unlock());
+  EXPECT_FALSE(locker.isLocked());
 
   EXPECT_EQ (0, unlink(TEST_DATA_TOPBUILDDIR "/test_lockfile"));
 }
diff --git a/src/lib/util/tests/interprocess_sync_null_unittest.cc b/src/lib/util/tests/interprocess_sync_null_unittest.cc
index 81cff76aaf..70e2b07f0e 100644
--- a/src/lib/util/tests/interprocess_sync_null_unittest.cc
+++ b/src/lib/util/tests/interprocess_sync_null_unittest.cc
@@ -24,28 +24,43 @@ TEST(InterprocessSyncNullTest, TestNull) {
   InterprocessSyncNull sync("test1");
   InterprocessSyncLocker locker(sync);
 
-  // All of these must always return true
-
+  // Check if the is_locked_ flag is set correctly during lock().
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.lock());
+  EXPECT_TRUE(locker.isLocked());
+
+  // lock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.lock());
 
+  // Check if the is_locked_ flag is set correctly during unlock().
+  EXPECT_TRUE(locker.isLocked());
   EXPECT_TRUE(locker.unlock());
+  EXPECT_FALSE(locker.isLocked());
+
+  // unlock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.unlock());
 
+  // Check if the is_locked_ flag is set correctly during tryLock().
+  EXPECT_FALSE(locker.isLocked());
   EXPECT_TRUE(locker.tryLock());
+  EXPECT_TRUE(locker.isLocked());
+
+  // tryLock() must always return true (this is called 4 times, just an
+  // arbitrary number)
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
   EXPECT_TRUE(locker.tryLock());
 
-  // Random order
-
+  // Random order (should all return true)
   EXPECT_TRUE(locker.unlock());
   EXPECT_TRUE(locker.lock());
   EXPECT_TRUE(locker.tryLock());