consolidate mutex locking to better debug deadlocks

src/conf.c

@@ -53,6 +53,7 @@
 #include "ll.h"
 #include "daapd.h"
 #include "os.h"
+#include "util.h"
 #include "webserver.h"
 #include "xml-rpc.h"
 
@@ -69,7 +70,6 @@ static LL_HANDLE conf_main=NULL;
 static LL_HANDLE conf_comments=NULL;
 
 static char *conf_main_file = NULL;
-static pthread_mutex_t conf_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 #define CONF_LINEBUFFER 1024
 
@@ -91,8 +91,6 @@ typedef struct _CONF_ELEMENTS {
 static int _conf_verify(LL_HANDLE pll);
 static LL_ITEM *_conf_fetch_item(LL_HANDLE pll, char *section, char *key);
 static int _conf_exists(LL_HANDLE pll, char *section, char *key);
-static void _conf_lock(void);
-static void _conf_unlock(void);
 static int _conf_write(FILE *fp, LL *pll, int sublevel, char *parent);
 static CONF_ELEMENTS *_conf_get_keyinfo(char *section, char *key);
 static int _conf_makedir(char *path, char *user);
@@ -247,30 +245,6 @@ CONF_ELEMENTS *_conf_get_keyinfo(char *section, char *key) {
     return found ? pcurrent : NULL;
 }
 
-/**
- * lock the conf mutex
- */
-void _conf_lock() {
-    int err;
-
-    if((err=pthread_mutex_lock(&conf_mutex))) {
-        DPRINTF(E_FATAL,L_CONF,"Cannot lock configuration mutex: %s\n",
-            strerror(err));
-    }
-}
-
-/**
- * unlock the conf mutex
- */
-void _conf_unlock() {
-    int err;
-
-    if((err = pthread_mutex_unlock(&conf_mutex))) {
-        DPRINTF(E_FATAL,L_CONF,"Cannot unlock configuration mutex %s\n",
-            strerror(err));
-    }
-}
-
 /**
  * fetch item based on section/term basis, rather than just a single
  * level deep, like ll_fetch_item does
@@ -850,7 +824,7 @@ int conf_read(char *file) {
     /*  Sanity check */
     if(_conf_verify(pllnew)) {
         DPRINTF(E_INF,L_CONF,"Loading new config file.\n");
-        _conf_lock();
+        util_mutex_lock(l_conf);
         _conf_apply(pllnew);
         if(conf_main) {
             ll_destroy(conf_main);
@@ -862,7 +836,7 @@ int conf_read(char *file) {
 
         conf_main = pllnew;
         conf_comments = pllcomment;
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
     } else {
         ll_destroy(pllnew);
         ll_destroy(pllcomment);
@@ -908,14 +882,14 @@ int conf_get_int(char *section, char *key, int dflt) {
     LL_ITEM *pitem;
     int retval;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     pitem = _conf_fetch_item(conf_main,section,key);
     if((!pitem) || (pitem->type != LL_TYPE_STRING)) {
         retval = dflt;
     } else {
         retval = atoi(pitem->value.as_string);
     }
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
 
     return retval;
 }
@@ -936,7 +910,7 @@ int conf_get_string(char *section, char *key, char *dflt, char *out, int *size)
     char *result;
     int len;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     pitem = _conf_fetch_item(conf_main,section,key);
     if((!pitem) || (pitem->type != LL_TYPE_STRING)) {
         result = dflt;
@@ -945,7 +919,7 @@ int conf_get_string(char *section, char *key, char *dflt, char *out, int *size)
     }
 
     if(!result) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return CONF_E_NOTFOUND;
     }
 
@@ -955,12 +929,12 @@ int conf_get_string(char *section, char *key, char *dflt, char *out, int *size)
         *size = len;
         strcpy(out,result);
     } else {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         *size = len;
         return CONF_E_OVERFLOW;
     }
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return CONF_E_SUCCESS;
 }
 
@@ -978,7 +952,7 @@ char *conf_alloc_string(char *section, char *key, char *dflt) {
     char *result;
     char *retval;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     pitem = _conf_fetch_item(conf_main,section,key);
     if((!pitem) || (pitem->type != LL_TYPE_STRING)) {
         result = dflt;
@@ -987,7 +961,7 @@ char *conf_alloc_string(char *section, char *key, char *dflt) {
     }
 
     if(result == NULL) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return NULL;
     }
 
@@ -996,7 +970,7 @@ char *conf_alloc_string(char *section, char *key, char *dflt) {
     if(!retval) {
         DPRINTF(E_FATAL,L_CONF,"Malloc error in conf_alloc_string\n");
     }
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return retval;
 }
 
@@ -1039,17 +1013,17 @@ int conf_set_string(char *section, char *key, char *value, int verify) {
     char *oldvalue=NULL;
     LL_ITEM *polditem;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
 
     /* verify the item */
     err=_conf_verify_element(section,key,value);
     if(err != CONF_E_SUCCESS) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return err;
     }
 
     if(verify) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return CONF_E_SUCCESS;
     }
 
@@ -1070,19 +1044,19 @@ int conf_set_string(char *section, char *key, char *value, int verify) {
         /* deleting the item */
         pitem = ll_fetch_item(conf_main,section);
         if(!pitem) {
-            _conf_unlock();
+            util_mutex_unlock(l_conf);
             return CONF_E_SUCCESS;
         }
 
         section_ll = pitem->value.as_ll;
         if(!section_ll) {
-            _conf_unlock();
+            util_mutex_unlock(l_conf);
             return CONF_E_SUCCESS; /* ?? deleting an already deleted item */
         }
 
         /* don't care about item... might already be gone! */
         ll_del_item(section_ll,key);
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return CONF_E_SUCCESS;
     }
 
@@ -1093,12 +1067,12 @@ int conf_set_string(char *section, char *key, char *value, int verify) {
             /* that subkey doesn't exist yet... */
             if((err = ll_create(&section_ll)) != LL_E_SUCCESS) {
                 DPRINTF(E_LOG,L_CONF,"Could not create linked list: %d\n",err);
-                _conf_unlock();
+                util_mutex_unlock(l_conf);
                 return CONF_E_UNKNOWN;
             }
             if((err=ll_add_ll(conf_main,section,section_ll)) != LL_E_SUCCESS) {
                 DPRINTF(E_LOG,L_CONF,"Error inserting new subkey: %d\n",err);
-                _conf_unlock();
+                util_mutex_unlock(l_conf);
                 return CONF_E_UNKNOWN;
             }
         } else {
@@ -1123,7 +1097,7 @@ int conf_set_string(char *section, char *key, char *value, int verify) {
             if((err = ll_add_string(section_ll,key,value)) != LL_E_SUCCESS) {
                 DPRINTF(E_LOG,L_CONF,"Error in conf_set_string: "
                         "(%s/%s)\n",section,key);
-                _conf_unlock();
+                util_mutex_unlock(l_conf);
                 return CONF_E_UNKNOWN;
             }
         }
@@ -1150,7 +1124,7 @@ int conf_set_string(char *section, char *key, char *value, int verify) {
         }
     }
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return conf_write();
 }
 
@@ -1163,14 +1137,14 @@ int conf_iswritable(void) {
     int retval = FALSE;
 
     /* don't want configfile reopened under us */
-    _conf_lock();
+    util_mutex_lock(l_conf);
 
     if(!conf_main_file)
         return FALSE;
 
     retval = !access(conf_main_file,W_OK);
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return retval;
 }
 
@@ -1186,12 +1160,12 @@ int conf_write(void) {
         return CONF_E_NOCONF;
     }
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     if((fp = fopen(conf_main_file,"w+")) != NULL) {
         retval = _conf_write(fp,conf_main,0,NULL);
         fclose(fp);
     }
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
 
     return retval ? CONF_E_SUCCESS : CONF_E_NOTWRITABLE;
 }
@@ -1302,11 +1276,11 @@ int _conf_write(FILE *fp, LL *pll, int sublevel, char *parent) {
 int conf_isset(char *section, char *key) {
     int retval = FALSE;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     if(_conf_fetch_item(conf_main,section,key)) {
         retval = TRUE;
     }
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
 
     return retval;
 }
@@ -1415,10 +1389,10 @@ char *conf_implode(char *section, char *key, char *delimiter) {
     int len;
     char *retval;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     pitem = _conf_fetch_item(conf_main,section,key);
     if((!pitem) || (pitem->type != LL_TYPE_LL)) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return NULL;
     }
 
@@ -1434,7 +1408,7 @@ char *conf_implode(char *section, char *key, char *delimiter) {
     }
 
     if(!count) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return NULL;
     }
 
@@ -1453,7 +1427,7 @@ char *conf_implode(char *section, char *key, char *delimiter) {
         }
     }
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return retval;
 }
 
@@ -1485,10 +1459,10 @@ int conf_get_array(char *section, char *key, char ***argvp) {
     int count;
     int len;
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
     pitem = _conf_fetch_item(conf_main,section,key);
     if((!pitem) || (pitem->type != LL_TYPE_LL)) {
-        _conf_unlock();
+        util_mutex_unlock(l_conf);
         return FALSE;
     }
 
@@ -1521,7 +1495,7 @@ int conf_get_array(char *section, char *key, char ***argvp) {
         count++;
     }
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
     return TRUE;
 }
 
@@ -1562,11 +1536,11 @@ int conf_xml_dump(WS_CONNINFO *pwsc) {
     pxml = xml_init(pwsc,1);
     xml_push(pxml,"config");
 
-    _conf_lock();
+    util_mutex_lock(l_conf);
 
     retval = _conf_xml_dump(pxml,conf_main,0,NULL);
 
-    _conf_unlock();
+    util_mutex_unlock(l_conf);
 
     xml_pop(pxml);
     xml_deinit(pxml);

src/err.c

@@ -63,7 +63,6 @@ static int err_debuglevel=0; /**< current debuglevel, set from command line with
 static int err_logdest=0; /**< current log destination */
 static char err_filename[PATH_MAX + 1];
 static FILE *err_file=NULL; /**< if logging to file, the handle of that file */
-static pthread_mutex_t err_mutex=PTHREAD_MUTEX_INITIALIZER; /**< for serializing log messages */
 static unsigned int err_debugmask=0xFFFFFFFF; /**< modules to debug, see \ref log_categories */
 static int err_truncate = 0;
 static int err_syslog_open = 0;
@@ -78,8 +77,6 @@ static char *err_categorylist[] = {
  * Forwards
  */
 
-static int _err_lock(void);
-static int _err_unlock(void);
 static uint32_t _err_get_threadid(void);
 
 
@@ -112,14 +109,12 @@ void err_reopen(void) {
     if(!(err_logdest & LOGDEST_LOGFILE))
         return;
 
-//    _err_lock();
     fclose(err_file);
     err_file = fopen(err_filename,"a");
     if(!err_file) {
         /* what to do when you lose your logging mechanism?  Keep
          * going?
          */
-        _err_unlock();
         err = errno;
         err_setdest(err_logdest & (~LOGDEST_LOGFILE));
         err_setdest(err_logdest | LOGDEST_SYSLOG);
@@ -128,7 +123,7 @@ void err_reopen(void) {
                 strerror(err));
         return;
     }
-//    _err_unlock();
+
     DPRINTF(E_LOG,L_MISC,"Rotated logs\n");
 }
 
@@ -162,7 +157,7 @@ void err_log(int level, unsigned int cat, char *fmt, ...)
     vsnprintf(errbuf, sizeof(errbuf), fmt, ap);
     va_end(ap);
 
-    _err_lock(); /* atomic file writes */
+    util_mutex_lock(l_err);
 
     if((err_logdest & LOGDEST_LOGFILE) && err_file) {
         tt_now=time(NULL);
@@ -189,7 +184,7 @@ void err_log(int level, unsigned int cat, char *fmt, ...)
         os_syslog(level,errbuf);
     }
 
-    _err_unlock();
+    util_mutex_unlock(l_err);
 
 #ifndef ERR_LEAN
     if(level < 2) { /* only event level fatals and log level */
@@ -207,9 +202,9 @@ void err_log(int level, unsigned int cat, char *fmt, ...)
  * simple get/set interface to debuglevel to avoid global
  */
 void err_setlevel(int level) {
-    _err_lock();
+    util_mutex_lock(l_err);
     err_debuglevel = level;
-    _err_unlock();
+    util_mutex_unlock(l_err);
 }
 
 /**
@@ -217,9 +212,10 @@ void err_setlevel(int level) {
  */
 int err_getlevel(void) {
     int level;
-    _err_lock();
+
+    util_mutex_lock(l_err);
     level = err_debuglevel;
-    _err_unlock();
+    util_mutex_unlock(l_err);
 
     return level;
 }
@@ -231,9 +227,9 @@ int err_getlevel(void) {
 int err_getdest(void) {
     int dest;
 
-    _err_lock();
+    util_mutex_lock(l_err);
     dest=err_logdest;
-    _err_unlock();
+    util_mutex_unlock(l_err);
 
     return dest;
 }
@@ -263,7 +259,6 @@ int err_setlogfile(char *file) {
     if(strcmp(file,err_filename) == 0)
         return TRUE;
 */
-//    _err_lock();
 
     if(err_file) {
         fclose(err_file);
@@ -285,7 +280,6 @@ int err_setlogfile(char *file) {
         result=FALSE;
     }
 
-//    _err_unlock();
     return result;
 }
 
@@ -299,7 +293,7 @@ void err_setdest(int destination) {
     if(err_logdest == destination)
         return;
 
-    _err_lock();
+    util_mutex_lock(l_err);
     if((err_logdest & LOGDEST_LOGFILE) &&
        (!(destination & LOGDEST_LOGFILE))) {
         /* used to be logging to file, not any more */
@@ -307,7 +301,7 @@ void err_setdest(int destination) {
     }
 
     err_logdest=destination;
-    _err_unlock();
+    util_mutex_unlock(l_err);
 }
 /**
  * Set the debug mask.  Given a comma separated list, this walks
@@ -327,7 +321,7 @@ extern int err_setdebugmask(char *list) {
     if(!str)
         return 0;
 
-    _err_lock();
+    util_mutex_lock(l_err);
     while(1) {
         token=strtok_r(str,",",&last);
         str=NULL;
@@ -342,7 +336,7 @@ extern int err_setdebugmask(char *list) {
             }
 
             if(!err_categorylist[index]) {
-                _err_unlock();
+                util_mutex_unlock(l_err);
                 DPRINTF(E_LOG,L_MISC,"Unknown module: %s\n",token);
                 free(tmpstr);
                 return 1;
@@ -352,44 +346,11 @@ extern int err_setdebugmask(char *list) {
         } else break; /* !token */
     }
 
-    _err_unlock();
+    util_mutex_unlock(l_err);
     DPRINTF(E_INF,L_MISC,"Debug mask is 0x%08x\n",err_debugmask);
     free(tmpstr);
 
     return 0;
 }
 
-/**
- * Lock the error mutex.  This is used to serialize
- * log messages, as well as protect access to the memory
- * list, when memory debugging is enabled.
- *
- * \returns 0 on success, otherwise -1 with errno set
- */
-int _err_lock(void) {
-    int err;
-
-    if((err=pthread_mutex_lock(&err_mutex))) {
-        errno=err;
-        return -1;
-    }
-
-    return 0;
-}
-
-/**
- * Unlock the error mutex
- *
- * \returns 0 on success, otherwise -1 with errno set
- */
-int _err_unlock(void) {
-    int err;
-
-    if((err=pthread_mutex_unlock(&err_mutex))) {
-        errno=err;
-        return -1;
-    }
-
-    return 0;
-}
 

src/util.c

@@ -6,6 +6,8 @@
 #  include "config.h"
 #endif
 
+#include <pthread.h>
+
 #ifdef HAVE_STDINT_H
 # include <stdint.h>
 #endif
@@ -21,10 +23,16 @@
 #include "err.h"
 #include "util.h"
 
+/* Globals */
+pthread_mutex_t util_locks[(int)l_last];
+pthread_mutex_t util_mutex = PTHREAD_MUTEX_INITIALIZER;
+int _util_initialized=0;
+
+
 /* Forwards */
 //int _util_xtoy(unsigned char *dbuffer, size_t dlen, unsigned char *sbuffer, size_t slen, char *from, char *to);
 void _util_hexdump(unsigned char *block, int len);
-
+void _util_mutex_init(void);
 
 /**
  * Simple hash generator
@@ -424,3 +432,47 @@ void _util_hexdump(unsigned char *block, int len) {
         fprintf(stderr,"%s\n",output);
     }
 }
+
+/**
+ * simple mutex wrapper for better debugging
+ */
+void util_mutex_lock(lock_t which) {
+    if(!_util_initialized)
+        _util_mutex_init();
+
+    pthread_mutex_lock(&util_locks[(int)which]);
+}
+
+/**
+ * simple mutex wrapper for better debugging
+ */
+void util_mutex_unlock(lock_t which) {
+    pthread_mutex_unlock(&util_locks[(int)which]);
+}
+
+/**
+ * mutex initializer.  This might should be done from the
+ * main thread.
+ */
+void _util_mutex_init(void) {
+    int err;
+    lock_t lock;
+
+    if((err = pthread_mutex_lock(&util_mutex))) {
+        fprintf(stderr,"Error locking mutex\n");
+        exit(-1);
+    }
+
+    if(!_util_initialized) {
+        /* now, walk through and manually initialize the mutexes */
+        for(lock=(lock_t)0; lock < l_last; lock++) {
+            if((err = pthread_mutex_init(&util_locks[(int)lock],NULL))) {
+                fprintf(stderr,"Error initializing mutex\n");
+                exit(-1);
+            }
+        }
+        _util_initialized=1;
+    }
+
+    pthread_mutex_unlock(&util_mutex);
+}

src/util.h

@@ -15,6 +15,18 @@
 
 #include <sys/types.h>
 
+typedef enum {
+    l_err,
+    l_conf,
+    l_plugin,
+    l_last
+} lock_t;
+
+/* debugging lock wrappers */
+
+extern void util_mutex_lock(lock_t which);
+extern void util_mutex_unlock(lock_t which);
+
 /* simple hashing functions */
 extern uint32_t util_djb_hash_block(unsigned char *data, uint32_t len);
 extern uint32_t util_djb_hash_str(char *str);