[asterisk-commits] murf: branch murf/bug6002 r105549 - in /team/murf/bug6002: include/asterisk/ ...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Sat Mar 1 12:56:24 CST 2008


Author: murf
Date: Sat Mar  1 12:56:23 2008
New Revision: 105549

URL: http://svn.digium.com/view/asterisk?view=rev&rev=105549
Log:
We can't just 'upgrade' a wrlock from read to write. We have to obtain the readlock, then release it, then get a write lock. THis is a bummer. So, I threw in some code to see if any other write lock was obtained between the release of the read lock and the obtaining of the write lock. If so, a warning is generated. To make this foolproof, perhaps we can re-run the merge operation under another readlock and then release and get the write lock again, until no writers intervene. At the moment, I don't see hoards of threads clamoring to mod the dialplan. Some debugs remain. Got rid of a problem or two with double-frees.

Modified:
    team/murf/bug6002/include/asterisk/pbx.h
    team/murf/bug6002/main/pbx.c

Modified: team/murf/bug6002/include/asterisk/pbx.h
URL: http://svn.digium.com/view/asterisk/team/murf/bug6002/include/asterisk/pbx.h?view=diff&rev=105549&r1=105548&r2=105549
==============================================================================
--- team/murf/bug6002/include/asterisk/pbx.h (original)
+++ team/murf/bug6002/include/asterisk/pbx.h Sat Mar  1 12:56:23 2008
@@ -965,6 +965,13 @@
 									 struct ast_context *bypass, struct pbx_find_info *q,
 									 const char *context, const char *exten, int priority,
 									 const char *label, const char *callerid, enum ext_match_t action);
+
+
+/* every time a write lock is obtained for contexts,
+   a counter is incremented. You can check this via the
+   following func */
+
+int ast_wrlock_contexts_version(void);
 	
 
 /* hashtable functions for contexts */

Modified: team/murf/bug6002/main/pbx.c
URL: http://svn.digium.com/view/asterisk/team/murf/bug6002/main/pbx.c?view=diff&rev=105549&r1=105548&r2=105549
==============================================================================
--- team/murf/bug6002/main/pbx.c (original)
+++ team/murf/bug6002/main/pbx.c Sat Mar  1 12:56:23 2008
@@ -48,6 +48,7 @@
 #include "asterisk/cdr.h"
 #include "asterisk/config.h"
 #include "asterisk/term.h"
+#include "asterisk/time.h"
 #include "asterisk/manager.h"
 #include "asterisk/ast_expr.h"
 #include "asterisk/linkedlists.h"
@@ -5192,7 +5193,7 @@
 	return tmp;
 }
 
-void __ast_context_destroy(struct ast_context *con, const char *registrar);
+void __ast_context_destroy(struct ast_context *list, struct ast_hashtab *contexttab, struct ast_context *con, const char *registrar);
 
 struct store_hint {
 	char *context;
@@ -5219,49 +5220,50 @@
 	   the current registrar, and copy them to the new context. If the new context does not
 	   exist, we'll create it "on demand". If no items are in this context to copy, then we'll
 	   only create the empty matching context if the old one meets the criteria */
-
-	exten_iter = ast_hashtab_start_traversal(context->root_table);
-	while ((exten_item=ast_hashtab_next(exten_iter))) {
-		if (new) {
-			new_exten_item = ast_hashtab_lookup(new->root_table, exten_item);
-		} else {
-			new_exten_item = NULL;
-		}
-		prio_iter = ast_hashtab_start_traversal(exten_item->peer_table);
-		while ((prio_item=ast_hashtab_next(prio_iter))) {
-			int res1;
-			
-			if (new_exten_item) {
-				new_prio_item = ast_hashtab_lookup(new_exten_item->peer_table, prio_item);
+	if (context->root_table) {
+		exten_iter = ast_hashtab_start_traversal(context->root_table);
+		while ((exten_item=ast_hashtab_next(exten_iter))) {
+			if (new) {
+				new_exten_item = ast_hashtab_lookup(new->root_table, exten_item);
 			} else {
-				new_prio_item = NULL;
-			}
-			if (strcmp(prio_item->registrar,registrar) == 0) {
-				continue;
-			}
-			/* make sure the new context exists, so we have somewhere to stick this exten/prio */
-			if (!new) {
-				new = ast_context_find_or_create(extcontexts, exttable, context->name, prio_item->registrar); /* a new context created via priority from a different context in the old dialplan, gets its registrar from the prio's registrar */
-			}
-			if (!new) {
-				ast_log(LOG_ERROR,"Could not allocate a new context for %s in merge_and_delete! Danger!\n", context->name);
-				return; /* no sense continuing. */
-			}
-			/* we will not replace existing entries in the new context with stuff from the old context.
-			   but, if this is because of some sort of registrar conflict, we ought to say something... */
-			res1 = ast_add_extension2(new, 0, prio_item->exten, prio_item->priority, prio_item->label, 
-									  prio_item->cidmatch, prio_item->app, prio_item->data, prio_item->datad, prio_item->registrar);
-			
-			if (!res1 && new_exten_item && new_prio_item){
-				ast_log(LOG_NOTICE,"Dropping old dialplan item %s/%s/%d [%s(%s)] (registrar=%s) due to conflict with new dialplan\n",
-						context->name, prio_item->exten, prio_item->priority, prio_item->app, (char*)prio_item->data, prio_item->registrar);
-			} else {
-				insert_count++;
-			}
-		}
-		ast_hashtab_end_traversal(prio_iter);
-	}
-	ast_hashtab_end_traversal(exten_iter);
+				new_exten_item = NULL;
+			}
+			prio_iter = ast_hashtab_start_traversal(exten_item->peer_table);
+			while ((prio_item=ast_hashtab_next(prio_iter))) {
+				int res1;
+				
+				if (new_exten_item) {
+					new_prio_item = ast_hashtab_lookup(new_exten_item->peer_table, prio_item);
+				} else {
+					new_prio_item = NULL;
+				}
+				if (strcmp(prio_item->registrar,registrar) == 0) {
+					continue;
+				}
+				/* make sure the new context exists, so we have somewhere to stick this exten/prio */
+				if (!new) {
+					new = ast_context_find_or_create(extcontexts, exttable, context->name, prio_item->registrar); /* a new context created via priority from a different context in the old dialplan, gets its registrar from the prio's registrar */
+				}
+				if (!new) {
+					ast_log(LOG_ERROR,"Could not allocate a new context for %s in merge_and_delete! Danger!\n", context->name);
+					return; /* no sense continuing. */
+				}
+				/* we will not replace existing entries in the new context with stuff from the old context.
+				   but, if this is because of some sort of registrar conflict, we ought to say something... */
+				res1 = ast_add_extension2(new, 0, prio_item->exten, prio_item->priority, prio_item->label, 
+										  prio_item->cidmatch, prio_item->app, prio_item->data, prio_item->datad, prio_item->registrar);
+				if (!res1 && new_exten_item && new_prio_item){
+					ast_log(LOG_NOTICE,"Dropping old dialplan item %s/%s/%d [%s(%s)] (registrar=%s) due to conflict with new dialplan\n",
+							context->name, prio_item->exten, prio_item->priority, prio_item->app, (char*)prio_item->data, prio_item->registrar);
+				} else {
+					prio_item->data = NULL; /* we pass the priority data from the old to the new */
+					insert_count++;
+				}
+			}
+			ast_hashtab_end_traversal(prio_iter);
+		}
+		ast_hashtab_end_traversal(exten_iter);
+	}
 	
 	if (!insert_count && !new && (strcmp(context->registrar, registrar) != 0 ||
 		  (strcmp(context->registrar, registrar) == 0 && context->refcount > 1))) {
@@ -5276,6 +5278,7 @@
 /* XXX this does not check that multiple contexts are merged */
 void ast_merge_contexts_and_delete(struct ast_context **extcontexts, struct ast_hashtab *exttable, const char *registrar)
 {
+	double ft;
 	struct ast_context *tmp, *oldcontextslist;
 	struct ast_hashtab *oldtable;
 	struct store_hints store = AST_LIST_HEAD_INIT_VALUE;
@@ -5294,16 +5297,29 @@
 	   in addition, the locks _must_ be taken in this order, because there are already
 	   other code paths that use this order
 	*/
-
+	
+	struct timeval begintime, writelocktime, endlocktime, enddeltime;
+	int wrlock_ver;
+	
+	begintime = ast_tvnow();
 	ast_rdlock_contexts();
 	iter = ast_hashtab_start_traversal(contexts_table);
 	while ((tmp=ast_hashtab_next(iter))) {
 		context_merge(extcontexts, exttable, tmp, registrar);
 	}
 	ast_hashtab_end_traversal(iter);
+	wrlock_ver = ast_wrlock_contexts_version();
+
+	ast_unlock_contexts(); /* this feels real retarded, but you must do
+							  what you must do If this isn't done, the following 
+						      wrlock is a guraranteed deadlock */
+	ast_wrlock_contexts();
+	if (ast_wrlock_contexts_version() > wrlock_ver+1) {
+		ast_log(LOG_WARNING,"Something changed the contexts in the middle of merging contexts!\n");
+	}
 	
-	ast_wrlock_contexts();
 	AST_RWLIST_WRLOCK(&hints);
+	writelocktime = ast_tvnow();
 
 	/* preserve all watchers for hints associated with this registrar */
 	AST_RWLIST_TRAVERSE(&hints, hint, list) {
@@ -5364,6 +5380,7 @@
 
 	AST_RWLIST_UNLOCK(&hints);
 	ast_unlock_contexts();
+	endlocktime = ast_tvnow();
 	
 	/* the old list and hashtab no longer are relevant, delete them while the rest of asterisk
 	   is now freely using the new stuff instead */
@@ -5376,6 +5393,23 @@
 		__ast_internal_context_destroy(tmp);
 		tmp = next;
 	}
+	enddeltime = ast_tvnow();
+	
+	ft = ast_tvdiff_us(writelocktime, begintime);
+	ft /= 1000000.0;
+	ast_log(LOG_NOTICE,"Time to scan old dialplan and merge leftovers back into the new: %g sec\n", ft);
+	
+	ft = ast_tvdiff_us(endlocktime, writelocktime);
+	ft /= 1000000.0;
+	ast_log(LOG_NOTICE,"Time to restore hints and swap in new dialplan: %g sec\n", ft);
+
+	ft = ast_tvdiff_us(enddeltime, endlocktime);
+	ft /= 1000000.0;
+	ast_log(LOG_NOTICE,"Time to delete the old dialplan: %g sec\n", ft);
+
+	ft = ast_tvdiff_us(enddeltime, begintime);
+	ft /= 1000000.0;
+	ast_log(LOG_NOTICE,"Total time merge_contexts_delete: %g sec\n", ft);
 	return;
 }
 
@@ -6757,6 +6791,8 @@
 	struct ast_ignorepat *ipi;
 	struct ast_context *tmp = con;
 
+	ast_log(LOG_NOTICE,"Destroying old version of %s\n", con->name);
+	
 	for (tmpi = tmp->includes; tmpi; ) { /* Free includes */
 		struct ast_include *tmpil = tmpi;
 		tmpi = tmpi->next;
@@ -6787,19 +6823,20 @@
 		e = e->next;
 		destroy_exten(el);
 	}
+	tmp->root = NULL;
 	ast_rwlock_destroy(&tmp->lock);
 	ast_free(tmp);
 }
 
 
-void __ast_context_destroy(struct ast_context *con, const char *registrar)
+void __ast_context_destroy(struct ast_context *list, struct ast_hashtab *contexttab, struct ast_context *con, const char *registrar)
 {
 	struct ast_context *tmp, *tmpl=NULL;
 	struct ast_exten *exten_item, *prio_item;
 
 
-	for (tmp = contexts; tmp; ) {
-		struct ast_context *next;	/* next starting point */
+	for (tmp = list; tmp; ) {
+		struct ast_context *next = NULL;	/* next starting point */
 		for (; tmp; tmpl = tmp, tmp = tmp->next) {
 			ast_debug(1, "check ctx %s %s\n", tmp->name, tmp->registrar);
 			if ( registrar || (con && strcasecmp(tmp->name, con->name)) )
@@ -6813,6 +6850,7 @@
 			/* then search thru and remove any extens that match registrar. */
 			struct ast_hashtab_iter *exten_iter;
 			struct ast_hashtab_iter *prio_iter;
+			ast_log(LOG_NOTICE, "Delete all,  context %s registrar=%s\n", tmp->name, tmp->registrar);
 
 			exten_iter = ast_hashtab_start_traversal(tmp->root_table);
 			while ((exten_item=ast_hashtab_next(exten_iter))) {
@@ -6821,6 +6859,9 @@
 					if (strcmp(prio_item->registrar,registrar) != 0) {
 						continue;
 					}
+					ast_log(LOG_NOTICE, "Remove %s/%s/%d, registrar=%s\n",
+							tmp->name, prio_item->exten, prio_item->priority, registrar);
+					
 					ast_context_remove_extension2(tmp, prio_item->exten, prio_item->priority, registrar);
 				}
 				ast_hashtab_end_traversal(prio_iter);
@@ -6830,7 +6871,7 @@
 			/* delete the context if it's registrar matches, is empty, has refcount of 1, */
 			if (strcmp(tmp->registrar, registrar) == 0 && tmp->refcount < 2 && !tmp->root) {
 				ast_debug(1, "delete ctx %s %s\n", tmp->name, tmp->registrar);
-				ast_hashtab_remove_this_object(contexts_table, tmp);
+				ast_hashtab_remove_this_object(contexttab, tmp);
 				
 				next = tmp->next;
 				if (tmpl)
@@ -6843,8 +6884,9 @@
 				__ast_internal_context_destroy(tmp);
 			}
 		} else if (con) {
+			ast_log(LOG_NOTICE, "Deleting context %s registrar=%s\n", tmp->name, tmp->registrar);
 			ast_debug(1, "delete ctx %s %s\n", tmp->name, tmp->registrar);
-			ast_hashtab_remove_this_object(contexts_table, tmp);
+			ast_hashtab_remove_this_object(contexttab, tmp);
 			
 			next = tmp->next;
 			if (tmpl)
@@ -6865,7 +6907,7 @@
 void ast_context_destroy(struct ast_context *con, const char *registrar)
 {
 	ast_wrlock_contexts();
-	__ast_context_destroy(con,registrar);
+	__ast_context_destroy(contexts, contexts_table, con,registrar);
 	ast_unlock_contexts();
 }
 
@@ -7679,12 +7721,21 @@
 
 	return 0;
 }
+static int conlock_wrlock_version = 0;
+
+int ast_wrlock_contexts_version(void)
+{
+	return conlock_wrlock_version;
+}
 
 /*
  * Lock context list functions ...
  */
 int ast_wrlock_contexts()
 {
+	int res = ast_rwlock_wrlock(&conlock);
+	if (!res)
+		ast_atomic_fetchadd_int(&conlock_wrlock_version, 1);
 	return ast_rwlock_wrlock(&conlock);
 }
 




More information about the asterisk-commits mailing list