From cc535d234f0ba6e8d1214ac1dfaaa6efec9cd3c5 Mon Sep 17 00:00:00 2001 From: Ron Pedde Date: Sun, 31 Dec 2006 21:28:47 +0000 Subject: [PATCH] consolidate mutex locking to better debug deadlocks --- src/conf.c | 100 ++++++++++++++++++++--------------------------------- src/err.c | 69 ++++++++---------------------------- src/util.c | 54 ++++++++++++++++++++++++++++- src/util.h | 12 +++++++ 4 files changed, 117 insertions(+), 118 deletions(-) diff --git a/src/conf.c b/src/conf.c index 1b32e297..aa82eafe 100644 --- a/src/conf.c +++ b/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(§ion_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); diff --git a/src/err.c b/src/err.c index f01e76ef..210b96f2 100644 --- a/src/err.c +++ b/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; -} diff --git a/src/util.c b/src/util.c index b3f0b3b8..271e3dc2 100644 --- a/src/util.c +++ b/src/util.c @@ -6,6 +6,8 @@ # include "config.h" #endif +#include + #ifdef HAVE_STDINT_H # include #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); +} diff --git a/src/util.h b/src/util.h index 4a05308a..23946e26 100644 --- a/src/util.h +++ b/src/util.h @@ -15,6 +15,18 @@ #include +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);