Merged revisions 176943 via svnmerge from

https://origsvn.digium.com/svn/asterisk/trunk

........
r176943 | murf | 2009-02-18 08:35:26 -0700 (Wed, 18 Feb 2009) | 45 lines


This patch fixes merge_contexts_and_delete so it does not deadlock when hints are present.

Reason: when I re-engineered the merge_and_delete func to
reduce its lock time, I failed to notice that the 
functions it calls still also do locking as before.
This leads to deadlocks on dialplan reloads, when
there are actually living, subscribed hints registered
in the system.

While the reporter come across this problem while using
AEL, I might note that these deadlocks should also happen
if extensions.conf were used.

Here I added these routines to pbx.c:

ast_add_extension_nolock
add_pri_lockopt
ast_add_extension2_lockopt
find_context
add_hint_nolock

All of the above routines are static and restricted
to be used only within pbx.c, and more specifically
within the merge_contexts_and_delete routine.

They are pretty much the same as their counterparts
except they don't lock contexts or hints.

Most of them now do the real work of their
name-alike, with optional locking via extra arguments,
and are called by their name-alike. The goal was to
have the original functions so they would behave
exactly as before.

Both PJ and I tested these fixes, and the deadlocking
problem is no longer encountered.

(closes issue #14357)
Reported by: pj
Patches:
      14357.diff uploaded by murf (license 17)
Tested by: pj, murf


........


git-svn-id: http://svn.digium.com/svn/asterisk/branches/1.6.0@176944 f38db490-d61c-443f-a65b-d21fe96a405b

1 files changed, 70 insertions, 16 deletions

diff --git a/main/pbx.c b/main/pbx.c
index 103d27815..1554beac7 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -339,6 +339,12 @@ static unsigned int hashtab_hash_extens(const void *obj);
 static unsigned int hashtab_hash_priority(const void *obj);
 static unsigned int hashtab_hash_labels(const void *obj);
 static void __ast_internal_context_destroy( struct ast_context *con);
+static int add_pri_lockopt(struct ast_context *con, struct ast_exten *tmp,
+	struct ast_exten *el, struct ast_exten *e, int replace, int lockhints);
+static int ast_add_extension2_lockopt(struct ast_context *con,
+	int replace, const char *extension, int priority, const char *label, const char *callerid,
+	const char *application, void *data, void (*datad)(void *),
+	const char *registrar, int lockconts, int lockhints);
 
 /* a func for qsort to use to sort a char array */
 static int compare_char(const void *a, const void *b)
@@ -3464,20 +3470,18 @@ int ast_extension_state_del(int id, ast_state_cb_type callback)
 	return ret;
 }
 
-/*! \brief Add hint to hint list, check initial extension state */
-static int ast_add_hint(struct ast_exten *e)
+
+/*! \brief Add hint to hint list, check initial extension state; the hints had better be WRLOCKED already! */
+static int ast_add_hint_nolock(struct ast_exten *e)
 {
 	struct ast_hint *hint;
 
 	if (!e)
 		return -1;
 
-	AST_RWLIST_WRLOCK(&hints);
-
 	/* Search if hint exists, do nothing */
 	AST_RWLIST_TRAVERSE(&hints, hint, list) {
 		if (hint->exten == e) {
-			AST_RWLIST_UNLOCK(&hints);
 			ast_debug(2, "HINTS: Not re-adding existing hint %s: %s\n", ast_get_extension_name(e), ast_get_extension_app(e));
 			return -1;
 		}
@@ -3486,7 +3490,6 @@ static int ast_add_hint(struct ast_exten *e)
 	ast_debug(2, "HINTS: Adding hint %s: %s\n", ast_get_extension_name(e), ast_get_extension_app(e));
 
 	if (!(hint = ast_calloc(1, sizeof(*hint)))) {
-		AST_RWLIST_UNLOCK(&hints);
 		return -1;
 	}
 	/* Initialize and insert new item at the top */
@@ -3494,10 +3497,21 @@ static int ast_add_hint(struct ast_exten *e)
 	hint->laststate = ast_extension_state2(e);
 	AST_RWLIST_INSERT_HEAD(&hints, hint, list);
 
-	AST_RWLIST_UNLOCK(&hints);
 	return 0;
 }
 
+/*! \brief Add hint to hint list, check initial extension state */
+static int ast_add_hint(struct ast_exten *e)
+{
+	int ret;
+
+	AST_RWLIST_WRLOCK(&hints);
+	ret = ast_add_hint_nolock(e);
+	AST_RWLIST_UNLOCK(&hints);
+	
+	return ret;
+}
+
 /*! \brief Change hint for an extension */
 static int ast_change_hint(struct ast_exten *oe, struct ast_exten *ne)
 {
@@ -5788,13 +5802,13 @@ void ast_merge_contexts_and_delete(struct ast_context **extcontexts, struct ast_
 	}
 	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_log(LOG_WARNING,"==================!!!!!!!!!!!!!!!Something changed the contexts in the middle of merging contexts!\n");
 	}
 	
 	AST_RWLIST_WRLOCK(&hints);
@@ -6549,6 +6563,17 @@ static int ext_strncpy(char *dst, const char *src, int len)
 static int add_pri(struct ast_context *con, struct ast_exten *tmp,
 	struct ast_exten *el, struct ast_exten *e, int replace)
 {
+	return add_pri_lockopt(con, tmp, el, e, replace, 1);
+}
+
+/*! 
+ * \brief add the extension in the priority chain.
+ * \retval 0 on success.
+ * \retval -1 on failure.
+*/
+static int add_pri_lockopt(struct ast_context *con, struct ast_exten *tmp,
+	struct ast_exten *el, struct ast_exten *e, int replace, int lockhints)
+{
 	struct ast_exten *ep;
 	struct ast_exten *eh=e;
 
@@ -6683,8 +6708,13 @@ static int add_pri(struct ast_context *con, struct ast_exten *tmp,
 			e->next = NULL;	/* e is no more at the head, so e->next must be reset */
 		}
 		/* And immediately return success. */
-		if (tmp->priority == PRIORITY_HINT)
-			 ast_add_hint(tmp);
+		if (tmp->priority == PRIORITY_HINT) {
+			if (lockhints) {
+				ast_add_hint(tmp);
+			} else {
+				ast_add_hint_nolock(tmp);
+			}
+		}
 	}
 	return 0;
 }
@@ -6719,6 +6749,19 @@ int ast_add_extension2(struct ast_context *con,
 	const char *application, void *data, void (*datad)(void *),
 	const char *registrar)
 {
+	return ast_add_extension2_lockopt(con, replace, extension, priority, label, callerid, application, data, datad, registrar, 1, 1);
+}
+
+/*! \brief
+ * Does all the work of ast_add_extension2, but adds two args, to determine if 
+ * context and hint locking should be done. In merge_and_delete, we need to do
+ * this without locking, as the locks are already held.
+ */
+static int ast_add_extension2_lockopt(struct ast_context *con,
+	int replace, const char *extension, int priority, const char *label, const char *callerid,
+	const char *application, void *data, void (*datad)(void *),
+	const char *registrar, int lockconts, int lockhints)
+{
 	/*
 	 * Sort extensions (or patterns) according to the rules indicated above.
 	 * These are implemented by the function ext_cmp()).
@@ -6790,7 +6833,9 @@ int ast_add_extension2(struct ast_context *con,
 	tmp->datad = datad;
 	tmp->registrar = registrar;
 
-	ast_wrlock_context(con);
+	if (lockconts) {
+		ast_wrlock_context(con);
+	}
 	
 	if (con->pattern_tree) { /* usually, on initial load, the pattern_tree isn't formed until the first find_exten; so if we are adding
 								an extension, and the trie exists, then we need to incrementally add this pattern to it. */
@@ -6823,7 +6868,9 @@ int ast_add_extension2(struct ast_context *con,
 	}
 	if (e && res == 0) { /* exact match, insert in the pri chain */
 		res = add_pri(con, tmp, el, e, replace);
-		ast_unlock_context(con);
+		if (lockconts) {
+			ast_unlock_context(con);
+		}
 		if (res < 0) {
 			errno = EEXIST;	/* XXX do we care ? */
 			return 0; /* XXX should we return -1 maybe ? */
@@ -6880,9 +6927,16 @@ int ast_add_extension2(struct ast_context *con,
 				
 		}
 		ast_hashtab_insert_safe(con->root_table, tmp);
-		ast_unlock_context(con);
-		if (tmp->priority == PRIORITY_HINT)
-			ast_add_hint(tmp);
+		if (lockconts) {
+			ast_unlock_context(con);
+		}
+		if (tmp->priority == PRIORITY_HINT) {
+			if (lockhints) {
+				ast_add_hint(tmp);
+			} else {
+				ast_add_hint_nolock(tmp);
+			}
+		}
 	}
 	if (option_debug) {
 		if (tmp->matchcid) {