[svn-commits] dvossel: branch 1.4 r292866 - /branches/1.4/channels/chan_local.c
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Mon Oct 25 14:05:12 CDT 2010
Author: dvossel
Date: Mon Oct 25 14:05:07 2010
New Revision: 292866
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=292866
Log:
This patch turns chan_local pvts into astobj2 objects.
chan_local does some dangerous things involving deadlock avoidance.
tech_pvt functions like hangup and queue_frame are provided with a
locked channel upon entry. Those functions are completely safe as
long as you don't attempt to give up that channel lock, but that is
impossible to guarantee due to the required deadlock avoidance necessary
to lock both the tech_pvt and both channels involved.
In the past, we have tried to account for this by doing things like
setting a "glare" flag that indicates what function should destroy the
pvt. This was used in local_hangup and local_queue_frame to decided
who should destroy the pvt if they collided in separate threads. I
have removed the need to do this by converting all chan_local tech_pvts
to astobj2. This means we can ref a pvt before deadlock avoidance
and not have to worry about that pvt possibly getting destroyed under
us. It also cleans up where we destroy the tech_pvt. The only unlink
from the tech_pvt container occurs in local_hangup now, which is where
it should occur.
Since there still may be thread collisions on some functions like
local_hangup after deadlock avoidance, I have added some checks to detect
those collisions and exit appropriately. I think this patch is going to
solve quite a bit of weirdness we have had with local channels in the past.
Modified:
branches/1.4/channels/chan_local.c
Modified: branches/1.4/channels/chan_local.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_local.c?view=diff&rev=292866&r1=292865&r2=292866
==============================================================================
--- branches/1.4/channels/chan_local.c (original)
+++ branches/1.4/channels/chan_local.c Mon Oct 25 14:05:07 2010
@@ -61,10 +61,18 @@
#include "asterisk/manager.h"
#include "asterisk/stringfields.h"
#include "asterisk/devicestate.h"
+#include "asterisk/astobj2.h"
static const char tdesc[] = "Local Proxy Channel Driver";
#define IS_OUTBOUND(a,b) (a == b->chan ? 1 : 0)
+
+/* right now we are treating the locals astobj2 container as a
+ * list. If there is ever a reason to make this more efficient
+ * increasing the bucket size would help. */
+static const int BUCKET_SIZE = 1;
+
+static struct ao2_container *locals;
static struct ast_channel *local_request(const char *type, int format, void *data, int *cause);
static int local_digit_begin(struct ast_channel *ast, char digit);
@@ -105,7 +113,6 @@
};
struct local_pvt {
- ast_mutex_t lock; /* Channel private lock */
unsigned int flags; /* Private flags */
char context[AST_MAX_CONTEXT]; /* Context to call */
char exten[AST_MAX_EXTENSION]; /* Extension to call */
@@ -117,14 +124,10 @@
AST_LIST_ENTRY(local_pvt) list; /* Next entity */
};
-#define LOCAL_GLARE_DETECT (1 << 0) /*!< Detect glare on hangup */
-#define LOCAL_CANCEL_QUEUE (1 << 1) /*!< Cancel queue */
-#define LOCAL_ALREADY_MASQED (1 << 2) /*!< Already masqueraded */
-#define LOCAL_LAUNCHED_PBX (1 << 3) /*!< PBX was launched */
-#define LOCAL_NO_OPTIMIZATION (1 << 4) /*!< Do not optimize using masquerading */
-#define LOCAL_MOH_PASSTHRU (1 << 5) /*!< Pass through music on hold start/stop frames */
-
-static AST_LIST_HEAD_STATIC(locals, local_pvt);
+#define LOCAL_ALREADY_MASQED (1 << 1) /*!< Already masqueraded */
+#define LOCAL_LAUNCHED_PBX (1 << 2) /*!< PBX was launched */
+#define LOCAL_NO_OPTIMIZATION (1 << 3) /*!< Do not optimize using masquerading */
+#define LOCAL_MOH_PASSTHRU (1 << 4) /*!< Pass through music on hold start/stop frames */
static int local_setoption(struct ast_channel *chan, int option, void * data, int datalen)
{
@@ -155,7 +158,7 @@
return -1;
}
- while (ast_mutex_trylock(&p->lock)) {
+ while (ao2_trylock(p)) {
ast_channel_unlock(chan);
sched_yield();
ast_channel_lock(chan);
@@ -170,14 +173,14 @@
otherchan = (write_info->chan == p->owner) ? p->chan : p->owner;
if (!otherchan || otherchan == write_info->chan) {
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
ast_channel_unlock(chan);
ast_log(LOG_WARNING, "Could not update other side of %s, other side went away.\n", chan->name);
return 0;
}
if (ast_channel_trylock(otherchan)) {
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
ast_channel_unlock(chan);
goto startover;
}
@@ -185,7 +188,7 @@
res = write_info->write_fn(otherchan, write_info->function, write_info->data, write_info->value);
ast_channel_unlock(otherchan);
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
ast_channel_unlock(chan);
return res;
@@ -218,16 +221,14 @@
return AST_DEVICE_UNKNOWN;
}
-/*!
- * \note Assumes the pvt is no longer in the pvts list
+/*! \brief queue a frame on a to either the p->owner or p->chan
+ *
+ * \note the local_pvt MUST have it's ref count bumped before entering this function and
+ * decremented after this function is called. This is a side effect of the deadlock
+ * avoidance that is necessary to lock 2 channels and a tech_pvt. Without a ref counted
+ * local_pvt, it is impossible to guarantee it will not be destroyed by another thread
+ * during deadlock avoidance.
*/
-static struct local_pvt *local_pvt_destroy(struct local_pvt *pvt)
-{
- ast_mutex_destroy(&pvt->lock);
- free(pvt);
- return NULL;
-}
-
static int local_queue_frame(struct local_pvt *p, int isoutbound, struct ast_frame *f,
struct ast_channel *us, int us_locked)
{
@@ -245,13 +246,10 @@
return 0;
}
- /* Set glare detection */
- ast_set_flag(p, LOCAL_GLARE_DETECT);
-
/* Ensure that we have both channels locked */
while (other && ast_channel_trylock(other)) {
int res;
- if ((res = ast_mutex_unlock(&p->lock))) {
+ if ((res = ao2_unlock(p))) {
ast_log(LOG_ERROR, "chan_local bug! '&p->lock' was not locked when entering local_queue_frame! (%s)\n", strerror(res));
return -1;
}
@@ -259,31 +257,17 @@
do {
if (ast_channel_unlock(us)) {
ast_log(LOG_ERROR, "chan_local bug! Our channel was not locked, yet arguments indicated that it was!!\n");
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
return -1;
}
usleep(1);
ast_channel_lock(us);
- } while (ast_mutex_trylock(&p->lock));
+ } while (ao2_trylock(p));
} else {
usleep(1);
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
}
other = isoutbound ? p->owner : p->chan;
- }
-
- /* Since glare detection only occurs within this function, and because
- * a pvt flag cannot be set without having the pvt lock, this is the only
- * location where we could detect a cancelling of the queue. */
- if (ast_test_flag(p, LOCAL_CANCEL_QUEUE)) {
- /* We had a glare on the hangup. Forget all this business,
- return and destroy p. */
- ast_mutex_unlock(&p->lock);
- p = local_pvt_destroy(p);
- if (other) {
- ast_channel_unlock(other);
- }
- return -1;
}
if (other) {
@@ -294,8 +278,6 @@
ast_channel_unlock(other);
}
- ast_clear_flag(p, LOCAL_GLARE_DETECT);
-
return 0;
}
@@ -308,16 +290,18 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
+ ao2_ref(p, 1);
isoutbound = IS_OUTBOUND(ast, p);
if (isoutbound) {
/* Pass along answer since somebody answered us */
struct ast_frame answer = { AST_FRAME_CONTROL, AST_CONTROL_ANSWER };
res = local_queue_frame(p, isoutbound, &answer, ast, 1);
- } else
+ } else {
ast_log(LOG_WARNING, "Huh? Local is being asked to answer?\n");
- if (!res)
- ast_mutex_unlock(&p->lock);
+ }
+ ao2_unlock(p);
+ ao2_ref(p, -1);
return res;
}
@@ -408,7 +392,8 @@
return -1;
/* Just queue for delivery to the other side */
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
+ ao2_ref(p, 1); /* ref for local_queue_frame */
isoutbound = IS_OUTBOUND(ast, p);
if (isoutbound && f && (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO))
check_bridge(p);
@@ -419,8 +404,9 @@
ast_log(LOG_DEBUG, "Not posting to queue since already masked on '%s'\n", ast->name);
res = 0;
}
- if (!res)
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
+ ao2_ref(p, -1);
+
return res;
}
@@ -431,11 +417,11 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
if ((p->owner != oldchan) && (p->chan != oldchan)) {
ast_log(LOG_WARNING, "Old channel wasn't %p but was %p/%p\n", oldchan, p->owner, p->chan);
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
return -1;
}
if (p->owner == oldchan)
@@ -446,12 +432,12 @@
/* Do not let a masquerade cause a Local channel to be bridged to itself! */
if (!ast_check_hangup(newchan) && (p->owner->_bridge == p->chan || p->chan->_bridge == p->owner)) {
ast_log(LOG_WARNING, "You can not bridge a Local channel to itself!\n");
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
ast_queue_hangup(newchan);
return -1;
}
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
return 0;
}
@@ -464,6 +450,8 @@
if (!p)
return -1;
+
+ ao2_ref(p, 1); /* ref for local_queue_frame */
/* If this is an MOH hold or unhold, do it on the Local channel versus real channel */
if (!ast_test_flag(p, LOCAL_MOH_PASSTHRU) && condition == AST_CONTROL_HOLD) {
@@ -472,14 +460,15 @@
ast_moh_stop(ast);
} else {
/* Queue up a frame representing the indication as a control frame */
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
isoutbound = IS_OUTBOUND(ast, p);
f.subclass = condition;
f.data = (void*)data;
f.datalen = datalen;
- if (!(res = local_queue_frame(p, isoutbound, &f, ast, 1)))
- ast_mutex_unlock(&p->lock);
- }
+ res = local_queue_frame(p, isoutbound, &f, ast, 1);
+ ao2_unlock(p);
+ }
+ ao2_ref(p, -1);
return res;
}
@@ -494,11 +483,13 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_ref(p, 1); /* ref for local_queue_frame */
+ ao2_lock(p);
isoutbound = IS_OUTBOUND(ast, p);
f.subclass = digit;
- if (!(res = local_queue_frame(p, isoutbound, &f, ast, 0)))
- ast_mutex_unlock(&p->lock);
+ res = local_queue_frame(p, isoutbound, &f, ast, 0);
+ ao2_unlock(p);
+ ao2_ref(p, -1);
return res;
}
@@ -513,12 +504,14 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
+ ao2_ref(p, 1); /* ref for local_queue_frame */
isoutbound = IS_OUTBOUND(ast, p);
f.subclass = digit;
f.len = duration;
- if (!(res = local_queue_frame(p, isoutbound, &f, ast, 0)))
- ast_mutex_unlock(&p->lock);
+ res = local_queue_frame(p, isoutbound, &f, ast, 0);
+ ao2_unlock(p);
+ ao2_ref(p, -1);
return res;
}
@@ -533,12 +526,14 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
+ ao2_ref(p, 1); /* ref for local_queue_frame */
isoutbound = IS_OUTBOUND(ast, p);
f.data = (char *) text;
f.datalen = strlen(text) + 1;
- if (!(res = local_queue_frame(p, isoutbound, &f, ast, 0)))
- ast_mutex_unlock(&p->lock);
+ res = local_queue_frame(p, isoutbound, &f, ast, 0);
+ ao2_unlock(p);
+ ao2_ref(p, -1);
return res;
}
@@ -552,13 +547,18 @@
if (!p)
return -1;
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
+ ao2_ref(p, 1); /* ref for local_queue_frame */
+
isoutbound = IS_OUTBOUND(ast, p);
f.subclass = subclass;
f.data = (char *)data;
f.datalen = datalen;
- if (!(res = local_queue_frame(p, isoutbound, &f, ast, 0)))
- ast_mutex_unlock(&p->lock);
+ res = local_queue_frame(p, isoutbound, &f, ast, 0);
+
+ ao2_unlock(p);
+ ao2_ref(p, -1);
+
return res;
}
@@ -567,14 +567,14 @@
static int local_call(struct ast_channel *ast, char *dest, int timeout)
{
struct local_pvt *p = ast->tech_pvt;
- int res;
+ int res = 0;
struct ast_var_t *varptr = NULL, *new;
size_t len, namelen;
if (!p)
return -1;
-
- ast_mutex_lock(&p->lock);
+
+ ao2_lock(p);
/*
* Note that cid_num and cid_name aren't passed in the ast_channel_alloc
@@ -597,7 +597,7 @@
if (!ast_exists_extension(NULL, p->chan->context, p->chan->exten, 1, p->owner->cid.cid_num)) {
ast_log(LOG_NOTICE, "No such extension/context %s@%s while calling Local channel\n", p->chan->exten, p->chan->context);
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
return -1;
}
@@ -618,7 +618,7 @@
if (!(res = ast_pbx_start(p->chan)))
ast_set_flag(p, LOCAL_LAUNCHED_PBX);
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
return res;
}
@@ -629,14 +629,17 @@
int isoutbound;
struct ast_frame f = { AST_FRAME_CONTROL, AST_CONTROL_HANGUP };
struct ast_channel *ochan = NULL;
- int glaredetect = 0, res = 0;
if (!p)
return -1;
- while (ast_mutex_trylock(&p->lock)) {
+ /* we MUST give the tech_pvt a ref here since we are unlocking the
+ * channel during deadlock avoidance. */
+ ao2_ref(p, 1);
+
+ while (ao2_trylock(p)) {
ast_channel_unlock(ast);
- usleep(1);
+ sched_yield();
ast_channel_lock(ast);
}
@@ -646,20 +649,30 @@
if ((status) && (p->owner)) {
/* Deadlock avoidance */
while (p->owner && ast_channel_trylock(p->owner)) {
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
if (p->chan) {
ast_channel_unlock(p->chan);
}
- usleep(1);
+ sched_yield();
if (p->chan) {
ast_channel_lock(p->chan);
}
- ast_mutex_lock(&p->lock);
+ ao2_lock(p);
}
if (p->owner) {
pbx_builtin_setvar_helper(p->owner, "CHANLOCALSTATUS", status);
ast_channel_unlock(p->owner);
}
+ }
+ if (!p->chan) {
+ /* chan was == to ast and was !NULL before deadlock avoidance started, if chan
+ * is NULL now, then we should bail because that channel
+ * hungup already. This is possible because we let go of the
+ * lock given to the ast channel passed to this function during
+ * deadlock avoidance. */
+ ao2_unlock(p);
+ ao2_ref(p, -1);
+ return 0;
}
p->chan = NULL;
ast_clear_flag(p, LOCAL_LAUNCHED_PBX);
@@ -667,53 +680,57 @@
} else {
ast_module_user_remove(p->u_owner);
while (p->chan && ast_channel_trylock(p->chan)) {
- ast_mutex_unlock(&p->lock);
+ ao2_unlock(p);
if (p->owner) {
ast_channel_unlock(p->owner);
}
- usleep(1);
+ sched_yield();
if (p->owner) {
ast_channel_lock(p->owner);
}
- ast_mutex_lock(&p->lock);
- }
-
- p->owner = NULL;
+ ao2_lock(p);
+ }
if (p->chan) {
ast_queue_hangup(p->chan);
ast_channel_unlock(p->chan);
}
- }
-
+
+ if (!p->owner) {
+ /* owner was == to ast and was !NULL before deadlock avoidance started, if
+ * owner is NULL now, then we should bail because that channel
+ * hungup already. This is possible because we let go of the
+ * lock given to the ast channel passed to this function during
+ * deadlock avoidance. */
+ ao2_unlock(p);
+ ao2_ref(p, -1);
+ return 0;
+ }
+ p->owner = NULL;
+ }
+
ast->tech_pvt = NULL;
-
+
if (!p->owner && !p->chan) {
- /* Okay, done with the private part now, too. */
- glaredetect = ast_test_flag(p, LOCAL_GLARE_DETECT);
- /* If we have a queue holding, don't actually destroy p yet, but
- let local_queue do it. */
- if (glaredetect)
- ast_set_flag(p, LOCAL_CANCEL_QUEUE);
+ ao2_unlock(p);
+
/* Remove from list */
- AST_LIST_LOCK(&locals);
- AST_LIST_REMOVE(&locals, p, list);
- AST_LIST_UNLOCK(&locals);
- ast_mutex_unlock(&p->lock);
- /* And destroy */
- if (!glaredetect) {
- p = local_pvt_destroy(p);
- }
+ ao2_unlink(locals, p);
+ ao2_ref(p, -1);
return 0;
}
- if (p->chan && !ast_test_flag(p, LOCAL_LAUNCHED_PBX))
+ if (p->chan && !ast_test_flag(p, LOCAL_LAUNCHED_PBX)) {
/* Need to actually hangup since there is no PBX */
ochan = p->chan;
- else
- res = local_queue_frame(p, isoutbound, &f, NULL, 1);
- if (!res)
- ast_mutex_unlock(&p->lock);
- if (ochan)
+ } else {
+ local_queue_frame(p, isoutbound, &f, NULL, 1);
+ }
+
+ ao2_unlock(p);
+ if (ochan) {
ast_hangup(ochan);
+ }
+
+ ao2_ref(p, -1);
return 0;
}
@@ -723,11 +740,11 @@
struct local_pvt *tmp = NULL;
char *c = NULL, *opts = NULL;
- if (!(tmp = ast_calloc(1, sizeof(*tmp))))
+ if (!(tmp = ao2_alloc(sizeof(*tmp), NULL))) {
return NULL;
+ }
/* Initialize private structure information */
- ast_mutex_init(&tmp->lock);
ast_copy_string(tmp->exten, data, sizeof(tmp->exten));
/* Look for options */
@@ -757,14 +774,11 @@
} else {
#endif
/* Add to list */
- AST_LIST_LOCK(&locals);
- AST_LIST_INSERT_HEAD(&locals, tmp, list);
- AST_LIST_UNLOCK(&locals);
+ ao2_link(locals, tmp);
#if 0
}
#endif
-
- return tmp;
+ return tmp; /* this is returned with a ref */
}
/*! \brief Start new local channel */
@@ -838,11 +852,9 @@
/* Allocate a new private structure and then Asterisk channel */
if ((p = local_alloc(data, format))) {
if (!(chan = local_new(p, AST_STATE_DOWN))) {
- AST_LIST_LOCK(&locals);
- AST_LIST_REMOVE(&locals, p, list);
- AST_LIST_UNLOCK(&locals);
- p = local_pvt_destroy(p);
- }
+ ao2_unlink(locals, p);
+ }
+ ao2_ref(p, -1); /* kill the ref from the alloc */
}
return chan;
@@ -852,20 +864,25 @@
static int locals_show(int fd, int argc, char **argv)
{
struct local_pvt *p = NULL;
+ struct ao2_iterator it;
if (argc != 3)
return RESULT_SHOWUSAGE;
- AST_LIST_LOCK(&locals);
- if (!AST_LIST_EMPTY(&locals)) {
- AST_LIST_TRAVERSE(&locals, p, list) {
- ast_mutex_lock(&p->lock);
- ast_cli(fd, "%s -- %s@%s\n", p->owner ? p->owner->name : "<unowned>", p->exten, p->context);
- ast_mutex_unlock(&p->lock);
- }
- } else
+
+ if (ao2_container_count(locals) == 0) {
ast_cli(fd, "No local channels in use\n");
- AST_LIST_UNLOCK(&locals);
+ return RESULT_SUCCESS;
+ }
+
+ it = ao2_iterator_init(locals, 0);
+ while ((p = ao2_iterator_next(&it))) {
+ ao2_lock(p);
+ ast_cli(fd, "%s -- %s@%s\n", p->owner ? p->owner->name : "<unowned>", p->exten, p->context);
+ ao2_unlock(p);
+ ao2_ref(p, -1);
+ }
+ ao2_iterator_destroy(&it);
return RESULT_SUCCESS;
}
@@ -880,14 +897,25 @@
show_locals_usage },
};
+static int locals_cmp_cb(void *obj, void *arg, int flags)
+{
+ return (obj == arg) ? CMP_MATCH : 0;
+}
+
/*! \brief Load module into PBX, register channel */
static int load_module(void)
{
+ if (!(locals = ao2_container_alloc(BUCKET_SIZE, NULL, locals_cmp_cb))) {
+ return -1;
+ }
+
/* Make sure we can register our channel type */
if (ast_channel_register(&local_tech)) {
ast_log(LOG_ERROR, "Unable to register channel class 'Local'\n");
- return -1;
- }
+ ao2_ref(locals, -1);
+ return -1;
+ }
+
ast_cli_register_multiple(cli_local, sizeof(cli_local) / sizeof(struct ast_cli_entry));
return 0;
}
@@ -896,21 +924,21 @@
static int unload_module(void)
{
struct local_pvt *p = NULL;
+ struct ao2_iterator it;
/* First, take us out of the channel loop */
ast_cli_unregister_multiple(cli_local, sizeof(cli_local) / sizeof(struct ast_cli_entry));
ast_channel_unregister(&local_tech);
- if (!AST_LIST_LOCK(&locals)) {
- /* Hangup all interfaces if they have an owner */
- AST_LIST_TRAVERSE(&locals, p, list) {
- if (p->owner)
- ast_softhangup(p->owner, AST_SOFTHANGUP_APPUNLOAD);
- }
- AST_LIST_UNLOCK(&locals);
- } else {
- ast_log(LOG_WARNING, "Unable to lock the monitor\n");
- return -1;
- }
+
+ it = ao2_iterator_init(locals, 0);
+ while ((p = ao2_iterator_next(&it))) {
+ if (p->owner) {
+ ast_softhangup(p->owner, AST_SOFTHANGUP_APPUNLOAD);
+ }
+ ao2_ref(p, -1);
+ }
+ ao2_iterator_destroy(&it);
+ ao2_ref(locals, -1);
return 0;
}
More information about the svn-commits
mailing list