From cd507f1461d531f71978cbe0f31e366cca64f336 Mon Sep 17 00:00:00 2001
From: vraatikka <vilho.raatikka@skysql.com>
Date: Thu, 10 Oct 2013 16:46:51 +0300
Subject: [PATCH] log_manager.cc 	blockbuf_get_writepos, when all
 existing buffers in blockbuf list are full, a new block buffer is created and
 added to the list. Adding to the list is done with mutex on hold. Mutex
 shouldn't be freed before the next iteration in while loop, which expects
 that bblist mutex is on hold. At the end of the function, removed an
 unnecessary debug assertion.

testlog.c
	Enabled more intensive write test. Replaced TRUE and FALSE with true, false, respectively.

skygw_utils.cc
	simple_mutex_unlock, added debug assertion to ensure that pthread mutex's user counter is always at least 0 (it goes negative in double free).
---
 log_manager/log_manager.cc |  4 ----
 log_manager/test/testlog.c | 19 +++++++++++--------
 utils/skygw_utils.cc       | 22 ++++++++++++++++++++--
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/log_manager/log_manager.cc b/log_manager/log_manager.cc
index 0772ceddc..8a721837f 100644
--- a/log_manager/log_manager.cc
+++ b/log_manager/log_manager.cc
@@ -771,9 +771,6 @@ static char* blockbuf_get_writepos(
                          */
                         bb_list->mlist_versno += 1;
                         ss_dassert(bb_list->mlist_versno%2 == 0);
-
-                        /** Unlock list */
-                        simple_mutex_unlock(&bb_list->mlist_mutex);
                     } else {
                         /**
                          * List and buffers are full.
@@ -865,7 +862,6 @@ static char* blockbuf_get_writepos(
         
         /** Unlock buffer */
         simple_mutex_unlock(&bb->bb_mutex);
-        ss_dassert(bb_list->mlist_mutex.sm_lock_thr != pthread_self());
         return pos;
 }
             
diff --git a/log_manager/test/testlog.c b/log_manager/test/testlog.c
index 3c495e787..a308cd55f 100644
--- a/log_manager/test/testlog.c
+++ b/log_manager/test/testlog.c
@@ -40,9 +40,12 @@ static void* thr_run_morelog(void* data);
 #define NTHR 256
 #define NITER 100
 
+#if 1
+#  define TEST1
+#endif
+
 #if 0
-#define TEST1
-#define TEST2
+#  define TEST2
 #endif
 
 #define TEST3
@@ -172,13 +175,13 @@ int main(int argc, char* argv[])
 
         do {
             skygw_message_wait(mes);
-            simple_mutex_lock(mtx, TRUE);
+            simple_mutex_lock(mtx, true);
             if (nactive > 0) {
                 simple_mutex_unlock(mtx);
                 continue;
             }
             break;
-        } while(TRUE);
+        } while(true);
 
         for (i=0; i<NTHR; i++) {
             pthread_join(thr[i]->tid, NULL);
@@ -223,13 +226,13 @@ int main(int argc, char* argv[])
         
         do {
             skygw_message_wait(mes);
-            simple_mutex_lock(mtx, TRUE);
+            simple_mutex_lock(mtx, true);
             if (nactive > 0) {
                 simple_mutex_unlock(mtx);
                 continue;
             }
             break;
-        } while(TRUE);
+        } while(true);
 
         for (i=0; i<NTHR; i++) {
             pthread_join(thr[i]->tid, NULL);
@@ -504,7 +507,7 @@ static void* thr_run(
         skygw_logmanager_init( 0, NULL);
         skygw_logmanager_init( 0, NULL);
         skygw_logmanager_done();
-        simple_mutex_lock(td->mtx, TRUE);
+        simple_mutex_lock(td->mtx, true);
         *td->nactive -= 1;
         simple_mutex_unlock(td->mtx);
         skygw_message_send(td->mes);
@@ -557,7 +560,7 @@ static void* thr_run_morelog(
                                   str,
                                   i);
         }
-        simple_mutex_lock(td->mtx, TRUE);
+        simple_mutex_lock(td->mtx, true);
         *td->nactive -= 1;
         simple_mutex_unlock(td->mtx);
         skygw_message_send(td->mes);        
diff --git a/utils/skygw_utils.cc b/utils/skygw_utils.cc
index 516f7bb0d..ef638b10b 100644
--- a/utils/skygw_utils.cc
+++ b/utils/skygw_utils.cc
@@ -1321,6 +1321,12 @@ int simple_mutex_lock(
 {
         int err;
 
+        /**
+         * Leaving the following serves as a reminder. It may assert
+         * any given time because sm_lock_thr is not protected.
+         *
+         * ss_dassert(sm->sm_lock_thr != pthread_self());
+         */
         if (block) {
                 err = pthread_mutex_lock(&sm->sm_mutex);
         } else {
@@ -1336,6 +1342,9 @@ int simple_mutex_lock(
                         strerror(errno));
                 perror("simple_mutex : ");
         } else {
+                /**
+                 * Note that these updates are not protected.
+                 */
                 sm->sm_locked = true;
                 sm->sm_lock_thr = pthread_self();
         }
@@ -1346,18 +1355,27 @@ int simple_mutex_unlock(
         simple_mutex_t* sm)
 {
         int err;
-
+        /**
+         * Leaving the following serves as a reminder. It may assert
+         * any given time because sm_lock_thr is not protected.
+         *
+         * ss_dassert(sm->sm_lock_thr == pthread_self());
+         */
         err = pthread_mutex_unlock(&sm->sm_mutex);
 
         if (err != 0) {
                 fprintf(stderr,
-                        "* Locking simple mutex %s failed due error "
+                        "* Unlocking simple mutex %s failed due error "
                         "%d, %s\n",
                         sm->sm_name, 
                         err,
                         strerror(errno));
                 perror("simple_mutex : ");
+                ss_dassert(sm->sm_mutex.__data.__nusers >= 0);
         } else {
+                /**
+                 * Note that these updates are not protected.
+                 */
                 sm->sm_locked = false;
                 sm->sm_lock_thr = 0;
         }