[asterisk-commits] jpeeler: branch 1.8 r302266 - in /branches/1.8: ./ main/pbx.c

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Tue Jan 18 14:20:12 CST 2011


Author: jpeeler
Date: Tue Jan 18 14:19:57 2011
New Revision: 302266

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=302266
Log:
Merged revisions 302265 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

........
  r302265 | jpeeler | 2011-01-18 14:13:52 -0600 (Tue, 18 Jan 2011) | 27 lines
  
  Convert device state callbacks to ao2 objects to fix a deadlock in chan_sip.
  
  Lock scenario presented here:
  Thread 1
   holds ast_rdlock_contexts &conlock
   holds handle_statechange hints
   holds handle_statechange hint
    waiting for cb_extensionstate
     Locked Here: chan_sip.c line 7428 (find_call)
  Thread 2
   holds handle_request_do &netlock
   holds find_call sip_pvt_ptr
    waiting for ast_rdlock_contexts &conlock
     Locked Here: pbx.c line 9911 (ast_rdlock_contexts)
  
  Chan_sip has an established locking order of locking the sip_pvt and then
  getting the context lock. So the as stated by the summary, the operations in
  thread 2 have been modified to no longer require the context lock.
  
  (closes issue #18310)
  Reported by: one47
  Patches: 
        statecbs_ao2.mk2.patch uploaded by one47 (license 23),
        modified by me
  
  Review: https://reviewboard.asterisk.org/r/1072/
........

Modified:
    branches/1.8/   (props changed)
    branches/1.8/main/pbx.c

Propchange: branches/1.8/
------------------------------------------------------------------------------
Binary property 'branch-1.6.2-merged' - no diff available.

Modified: branches/1.8/main/pbx.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/pbx.c?view=diff&rev=302266&r1=302265&r2=302266
==============================================================================
--- branches/1.8/main/pbx.c (original)
+++ branches/1.8/main/pbx.c Tue Jan 18 14:19:57 2011
@@ -933,7 +933,7 @@
 struct ast_hint {
 	struct ast_exten *exten;	/*!< Extension */
 	int laststate;			/*!< Last known state */
-	AST_LIST_HEAD_NOLOCK(, ast_state_cb) callbacks; /*!< Callback list for this extension */
+	struct ao2_container *callbacks; /*!< Callback container for this extension */
 };
 
 /* --- Hash tables of various objects --------*/
@@ -1190,8 +1190,7 @@
 */
 static struct ao2_container *hints;
 
-/* XXX TODO Convert this to an astobj2 container, too. */
-static AST_LIST_HEAD_NOLOCK_STATIC(statecbs, ast_state_cb);
+static struct ao2_container *statecbs;
 
 #ifdef CONTEXT_DEBUG
 
@@ -4233,15 +4232,15 @@
 	struct ast_str *str;
 	struct statechange *sc = datap;
 	struct ao2_iterator i;
+	struct ao2_iterator cb_iter;
 
 	if (!(str = ast_str_create(1024))) {
 		return -1;
 	}
 
-
 	i = ao2_iterator_init(hints, 0);
 	for (hint = ao2_iterator_next(&i); hint; ao2_ref(hint, -1), hint = ao2_iterator_next(&i)) {
-		struct ast_state_cb *cblist;
+		struct ast_state_cb *state_cb;
 		char *cur, *parse;
 		int state;
 
@@ -4267,7 +4266,6 @@
 
 		/* Device state changed since last check - notify the watchers */
 
-		ast_rdlock_contexts();
 		ao2_lock(hints);
 		ao2_lock(hint);
 
@@ -4275,24 +4273,26 @@
 			/* the extension has been destroyed */
 			ao2_unlock(hint);
 			ao2_unlock(hints);
-			ast_unlock_contexts();
 			continue;
 		}
 
 		/* For general callbacks */
-		AST_LIST_TRAVERSE(&statecbs, cblist, entry) {
-			cblist->callback(hint->exten->parent->name, hint->exten->exten, state, cblist->data);
-		}
+		cb_iter = ao2_iterator_init(statecbs, 0);
+		for (state_cb = ao2_iterator_next(&cb_iter); state_cb; ao2_ref(state_cb, -1), state_cb = ao2_iterator_next(&cb_iter)) {
+			state_cb->callback(hint->exten->parent->name, hint->exten->exten, state, state_cb->data);
+		}
+		ao2_iterator_destroy(&cb_iter);
 
 		/* For extension callbacks */
-		AST_LIST_TRAVERSE(&hint->callbacks, cblist, entry) {
-			cblist->callback(hint->exten->parent->name, hint->exten->exten, state, cblist->data);
-		}
+		cb_iter = ao2_iterator_init(hint->callbacks, 0);
+		for (state_cb = ao2_iterator_next(&cb_iter); state_cb; ao2_ref(state_cb, -1), state_cb = ao2_iterator_next(&cb_iter)) {
+			state_cb->callback(hint->exten->parent->name, hint->exten->exten, state, state_cb->data);
+		}
+		ao2_iterator_destroy(&cb_iter);
 
 		hint->laststate = state;	/* record we saw the change */
 		ao2_unlock(hint);
 		ao2_unlock(hints);
-		ast_unlock_contexts();
 	}
 	ao2_iterator_destroy(&i);
 	ast_free(str);
@@ -4305,31 +4305,32 @@
 			    ast_state_cb_type callback, void *data)
 {
 	struct ast_hint *hint;
-	struct ast_state_cb *cblist;
+	struct ast_state_cb *state_cb;
 	struct ast_exten *e;
 
 	/* If there's no context and extension:  add callback to statecbs list */
 	if (!context && !exten) {
 		ao2_lock(hints);
 
-		AST_LIST_TRAVERSE(&statecbs, cblist, entry) {
-			if (cblist->callback == callback) {
-				cblist->data = data;
-				ao2_unlock(hints);
-				return 0;
-			}
+		state_cb = ao2_find(statecbs, callback, 0);
+		if (state_cb) {
+			state_cb->data = data;
+			ao2_ref(state_cb, -1);
+			ao2_unlock(hints);
+			return 0;
 		}
 
 		/* Now insert the callback */
-		if (!(cblist = ast_calloc(1, sizeof(*cblist)))) {
+		if (!(state_cb = ao2_alloc(sizeof(*state_cb), NULL))) {
 			ao2_unlock(hints);
 			return -1;
 		}
-		cblist->id = 0;
-		cblist->callback = callback;
-		cblist->data = data;
-
-		AST_LIST_INSERT_HEAD(&statecbs, cblist, entry);
+		state_cb->id = 0;
+		state_cb->callback = callback;
+		state_cb->data = data;
+
+		ao2_link(statecbs, state_cb);
+		ao2_ref(state_cb, -1);
 
 		ao2_unlock(hints);
 		return 0;
@@ -4366,35 +4367,35 @@
 	}
 
 	/* Now insert the callback in the callback list  */
-	if (!(cblist = ast_calloc(1, sizeof(*cblist)))) {
+	if (!(state_cb = ao2_alloc(sizeof(*state_cb), NULL))) {
 		ao2_ref(hint, -1);
 		return -1;
 	}
 
-	cblist->id = stateid++;		/* Unique ID for this callback */
-	cblist->callback = callback;	/* Pointer to callback routine */
-	cblist->data = data;		/* Data for the callback */
+	state_cb->id = stateid++;		/* Unique ID for this callback */
+	state_cb->callback = callback;	/* Pointer to callback routine */
+	state_cb->data = data;		/* Data for the callback */
 
 	ao2_lock(hint);
-	AST_LIST_INSERT_HEAD(&hint->callbacks, cblist, entry);
+	ao2_link(hint->callbacks, state_cb);
+	ao2_ref(state_cb, -1);
 	ao2_unlock(hint);
 
 	ao2_ref(hint, -1);
 
-	return cblist->id;
+	return state_cb->id;
 }
 
 /*! \brief Remove a watcher from the callback list */
 static int find_hint_by_cb_id(void *obj, void *arg, int flags)
 {
+	struct ast_state_cb *state_cb;
 	const struct ast_hint *hint = obj;
 	int *id = arg;
-	struct ast_state_cb *cb;
-
-	AST_LIST_TRAVERSE(&hint->callbacks, cb, entry) {
-		if (cb->id == *id) {
-			return CMP_MATCH | CMP_STOP;
-		}
+
+	if ((state_cb = ao2_find(hint->callbacks, id, 0))) {
+		ao2_ref(state_cb, -1);
+		return CMP_MATCH | CMP_STOP;
 	}
 
 	return 0;
@@ -4412,13 +4413,11 @@
 
 	if (!id) {	/* id == 0 is a callback without extension */
 		ao2_lock(hints);
-		AST_LIST_TRAVERSE_SAFE_BEGIN(&statecbs, p_cur, entry) {
-			if (p_cur->callback == callback) {
-				AST_LIST_REMOVE_CURRENT(entry);
-				break;
-			}
-		}
-		AST_LIST_TRAVERSE_SAFE_END;
+		p_cur = ao2_find(statecbs, callback, OBJ_UNLINK);
+		if (p_cur) {
+			ret = 0;
+			ao2_ref(p_cur, -1);
+		}
 		ao2_unlock(hints);
 	} else { /* callback with extension, find the callback based on ID */
 		struct ast_hint *hint;
@@ -4427,27 +4426,27 @@
 
 		if (hint) {
 			ao2_lock(hint);
-			AST_LIST_TRAVERSE_SAFE_BEGIN(&hint->callbacks, p_cur, entry) {
-				if (p_cur->id == id) {
-					AST_LIST_REMOVE_CURRENT(entry);
-					ret = 0;
-					break;
-				}
-			}
-			AST_LIST_TRAVERSE_SAFE_END;
-
+			p_cur = ao2_find(hint->callbacks, &id, OBJ_UNLINK);
+			if (p_cur) {
+				ret = 0;
+				ao2_ref(p_cur, -1);
+			}
 			ao2_unlock(hint);
 			ao2_ref(hint, -1);
 		}
 	}
 
-	if (p_cur) {
-		ast_free(p_cur);
-	}
-
 	return ret;
 }
 
+
+static int hint_id_cmp(void *obj, void *arg, int flags)
+{
+	const struct ast_state_cb *cb = obj;
+	int *id = arg;
+
+	return (cb->id == *id) ? CMP_MATCH | CMP_STOP : 0;
+}
 
 /*! \brief Add hint to hint list, check initial extension state */
 static int ast_add_hint(struct ast_exten *e)
@@ -4471,6 +4470,9 @@
 	if (!(hint = ao2_alloc(sizeof(*hint), NULL))) {
 		return -1;
 	}
+	if (!(hint->callbacks = ao2_container_alloc(1, NULL, hint_id_cmp))) {
+		return -1;
+	}
 
 	/* Initialize and insert new item at the top */
 	hint->exten = e;
@@ -4507,7 +4509,7 @@
 {
 	/* Cleanup the Notifys if hint is removed */
 	struct ast_hint *hint;
-	struct ast_state_cb *cblist;
+	struct ast_state_cb *state_cb;
 
 	if (!e) {
 		return -1;
@@ -4520,15 +4522,16 @@
 	}
 	ao2_lock(hint);
 
-	while ((cblist = AST_LIST_REMOVE_HEAD(&hint->callbacks, entry))) {
+	while ((state_cb = ao2_callback(hint->callbacks, OBJ_UNLINK, NULL, NULL))) {
 		/* Notify with -1 and remove all callbacks */
-		cblist->callback(hint->exten->parent->name, hint->exten->exten,
-			AST_EXTENSION_DEACTIVATED, cblist->data);
-		ast_free(cblist);
+		state_cb->callback(hint->exten->parent->name, hint->exten->exten,
+			AST_EXTENSION_DEACTIVATED, state_cb->data);
+		ao2_ref(state_cb, -1);
 	}
 
 	hint->exten = NULL;
 	ao2_unlink(hints, hint);
+	ao2_ref(hint->callbacks, -1);
 	ao2_unlock(hint);
 	ao2_ref(hint, -1);
 
@@ -5748,7 +5751,6 @@
 	struct ast_hint *hint;
 	int num = 0;
 	int watchers;
-	struct ast_state_cb *watcher;
 	struct ao2_iterator i;
 
 	switch (cmd) {
@@ -5772,10 +5774,7 @@
 	i = ao2_iterator_init(hints, 0);
 	for (hint = ao2_iterator_next(&i); hint; ao2_ref(hint, -1), hint = ao2_iterator_next(&i)) {
 
-		watchers = 0;
-		AST_LIST_TRAVERSE(&hint->callbacks, watcher, entry) {
-			watchers++;
-		}
+		watchers = ao2_container_count(hint->callbacks);
 		ast_cli(a->fd, "   %20s@%-20.20s: %-20.20s  State:%-15.15s Watchers %2d\n",
 			ast_get_extension_name(hint->exten),
 			ast_get_context_name(ast_get_extension_context(hint->exten)),
@@ -5825,7 +5824,6 @@
 	struct ast_hint *hint;
 	int watchers;
 	int num = 0, extenlen;
-	struct ast_state_cb *watcher;
 	struct ao2_iterator i;
 
 	switch (cmd) {
@@ -5852,10 +5850,7 @@
 	for (hint = ao2_iterator_next(&i); hint; ao2_unlock(hint), ao2_ref(hint, -1), hint = ao2_iterator_next(&i)) {
 		ao2_lock(hint);
 		if (!strncasecmp(hint->exten ? ast_get_extension_name(hint->exten) : "", a->argv[3], extenlen)) {
-			watchers = 0;
-			AST_LIST_TRAVERSE(&hint->callbacks, watcher, entry) {
-				watchers++;
-			}
+			watchers = ao2_container_count(hint->callbacks);
 			ast_cli(a->fd, "   %20s@%-20.20s: %-20.20s  State:%-15.15s Watchers %2d\n",
 				ast_get_extension_name(hint->exten),
 				ast_get_context_name(ast_get_extension_context(hint->exten)),
@@ -7083,7 +7078,8 @@
 	/* preserve all watchers for hints */
 	i = ao2_iterator_init(hints, AO2_ITERATOR_DONTLOCK);
 	for (hint = ao2_iterator_next(&i); hint; ao2_ref(hint, -1), hint = ao2_iterator_next(&i)) {
-		if (!AST_LIST_EMPTY(&hint->callbacks)) {
+		if (ao2_container_count(hint->callbacks)) {
+
 			length = strlen(hint->exten->exten) + strlen(hint->exten->parent->name) + 2 + sizeof(*this);
 			if (!(this = ast_calloc(1, length))) {
 				continue;
@@ -7095,8 +7091,12 @@
 				continue;
 			}
 
-			/* this removes all the callbacks from the hint into this. */
-			AST_LIST_APPEND_LIST(&this->callbacks, &hint->callbacks, entry);
+			/* this removes all the callbacks from the hint into 'this'. */
+			while ((thiscb = ao2_callback(hint->callbacks, OBJ_UNLINK, NULL, NULL))) {
+				AST_LIST_INSERT_TAIL(&this->callbacks, thiscb, entry);
+				/* We intentionally do not unref thiscb to account for the non-ao2 reference in this->callbacks */
+			}
+
 			this->laststate = hint->laststate;
 			this->context = this->data;
 			strcpy(this->data, hint->exten->parent->name);
@@ -7138,11 +7138,14 @@
 			/* this hint has been removed, notify the watchers */
 			while ((thiscb = AST_LIST_REMOVE_HEAD(&this->callbacks, entry))) {
 				thiscb->callback(this->context, this->exten, AST_EXTENSION_REMOVED, thiscb->data);
-				ast_free(thiscb);
+				ao2_ref(thiscb, -1);  /* Ref that we added when putting into this->callbacks */
 			}
 		} else {
 			ao2_lock(hint);
-			AST_LIST_APPEND_LIST(&hint->callbacks, &this->callbacks, entry);
+			while ((thiscb = AST_LIST_REMOVE_HEAD(&this->callbacks, entry))) {
+				ao2_link(hint->callbacks, thiscb);
+				ao2_ref(thiscb, -1);  /* Ref that we added when putting into this->callbacks */
+			}
 			hint->laststate = this->laststate;
 			ao2_unlock(hint);
 		}
@@ -9821,7 +9824,6 @@
 	struct ast_data *data_hint;
 	struct ast_hint *hint;
 	int watchers;
-	struct ast_state_cb *watcher;
 	struct ao2_iterator i;
 
 	if (ao2_container_count(hints) == 0) {
@@ -9830,10 +9832,7 @@
 
 	i = ao2_iterator_init(hints, 0);
 	for (hint = ao2_iterator_next(&i); hint; ao2_ref(hint, -1), hint = ao2_iterator_next(&i)) {
-		watchers = 0;
-		AST_LIST_TRAVERSE(&hint->callbacks, watcher, entry) {
-			watchers++;
-		}
+		watchers = ao2_container_count(hint->callbacks);
 		data_hint = ast_data_add_node(data_root, "hint");
 		if (!data_hint) {
 			continue;
@@ -10240,9 +10239,18 @@
 	return (hint->exten == exten) ? CMP_MATCH | CMP_STOP : 0;
 }
 
+static int statecbs_cmp(void *obj, void *arg, int flags)
+{
+	const struct ast_state_cb *state_cb = obj;
+	const struct ast_state_cb *callback = arg;
+
+	return (state_cb == callback) ? CMP_MATCH | CMP_STOP : 0;
+}
+
 int ast_pbx_init(void)
 {
 	hints = ao2_container_alloc(HASH_EXTENHINT_SIZE, hint_hash, hint_cmp);
-
-	return hints ? 0 : -1;
-}
+	statecbs = ao2_container_alloc(HASH_EXTENHINT_SIZE, NULL, statecbs_cmp);
+
+	return (hints && statecbs) ? 0 : -1;
+}




More information about the asterisk-commits mailing list