[svn-commits] rmudgett: branch 1.8 r348940 - in /branches/1.8: channels/ include/asterisk/ ...
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Thu Dec 22 20:09:23 CST 2011
Author: rmudgett
Date: Thu Dec 22 20:09:16 2011
New Revision: 348940
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=348940
Log:
Fix extension state callback references in chan_sip.
Chan_sip gives a dialog reference to the extension state callback and
assumes that when ast_extension_state_del() returns, the callback cannot
happen anymore. Chan_sip then reduces the dialog reference count
associated with the callback. Recent changes (ASTERISK-17760) have
resulted in the potential for the callback to happen after
ast_extension_state_del() has returned. For chan_sip, this could be very
bad because the dialog pointer could have already been destroyed.
* Added ast_extension_state_add_destroy() so chan_sip can account for the
sip_pvt reference given to the extension state callback when the extension
state callback is deleted.
* Fix pbx.c awkward statecbs handling in ast_extension_state_add_destroy()
and handle_statechange() now that the struct ast_state_cb has a destructor
to call.
* Ensure that ast_extension_state_add_destroy() will never return -1 or 0
for a successful registration.
* Fixed pbx.c statecbs_cmp() to compare the correct information. The
passed in value to compare is a change_cb function pointer not an object
pointer.
* Make pbx.c ast_merge_contexts_and_delete() not perform callbacks with
AST_EXTENSION_REMOVED with locks held. Chan_sip is notorious for
deadlocking when those locks are held during the callback.
* Removed unused lock declaration for the pbx.c store_hints list.
(closes issue ASTERISK-18844)
Reported by: rmudgett
Review: https://reviewboard.asterisk.org/r/1635/
Modified:
branches/1.8/channels/chan_sip.c
branches/1.8/include/asterisk/pbx.h
branches/1.8/main/pbx.c
Modified: branches/1.8/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/channels/chan_sip.c?view=diff&rev=348940&r1=348939&r2=348940
==============================================================================
--- branches/1.8/channels/chan_sip.c (original)
+++ branches/1.8/channels/chan_sip.c Thu Dec 22 20:09:16 2011
@@ -2958,10 +2958,9 @@
}
dialog->registry = registry_unref(dialog->registry, "delete dialog->registry");
}
- if (dialog->stateid > -1) {
- ast_extension_state_del(dialog->stateid, NULL);
- dialog_unref(dialog, "removing extension_state, should unref the associated dialog ptr that was stored there.");
- dialog->stateid = -1; /* shouldn't we 'zero' this out? */
+ if (dialog->stateid != -1) {
+ ast_extension_state_del(dialog->stateid, cb_extensionstate);
+ dialog->stateid = -1;
}
/* Remove link from peer to subscription of MWI */
if (dialog->relatedpeer && dialog->relatedpeer->mwipvt == dialog) {
@@ -14327,6 +14326,13 @@
}
}
+static void cb_extensionstate_destroy(int id, void *data)
+{
+ struct sip_pvt *p = data;
+
+ dialog_unref(p, "the extensionstate containing this dialog ptr was destroyed");
+}
+
/*! \brief Callback for the devicestate notification (SUBSCRIBE) support subsystem
\note If you add an "hint" priority to the extension in the dial plan,
you will get notifications on device state changes */
@@ -14341,7 +14347,6 @@
case AST_EXTENSION_REMOVED: /* Extension is gone */
sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT); /* Delete subscription in 32 secs */
ast_verb(2, "Extension state: Watcher for hint %s %s. Notify User %s\n", exten, state == AST_EXTENSION_DEACTIVATED ? "deactivated" : "removed", p->username);
- p->stateid = -1;
p->subscribed = NONE;
append_history(p, "Subscribestatus", "%s", state == AST_EXTENSION_REMOVED ? "HintRemoved" : "Deactivated");
break;
@@ -24346,7 +24351,7 @@
{
int gotdest = 0;
int res = 0;
- int firststate = AST_EXTENSION_REMOVED;
+ int firststate;
struct sip_peer *authpeer = NULL;
const char *eventheader = get_header(req, "Event"); /* Get Event package name */
int resubscribe = (p->subscribed != NONE) && !req->ignore;
@@ -24638,12 +24643,15 @@
/* Add subscription for extension state from the PBX core */
if (p->subscribed != MWI_NOTIFICATION && p->subscribed != CALL_COMPLETION && !resubscribe) {
- if (p->stateid > -1) {
+ if (p->stateid != -1) {
ast_extension_state_del(p->stateid, cb_extensionstate);
- /* we need to dec the refcount, now that the extensionstate is removed */
- dialog_unref(p, "the extensionstate containing this dialog ptr was deleted");
- }
- p->stateid = ast_extension_state_add(p->context, p->exten, cb_extensionstate, dialog_ref(p,"copying dialog ptr into extension state struct"));
+ }
+ dialog_ref(p, "copying dialog ptr into extension state struct");
+ p->stateid = ast_extension_state_add_destroy(p->context, p->exten,
+ cb_extensionstate, cb_extensionstate_destroy, p);
+ if (p->stateid == -1) {
+ dialog_unref(p, "copying dialog ptr into extension state struct failed");
+ }
}
if (!req->ignore && p)
Modified: branches/1.8/include/asterisk/pbx.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/pbx.h?view=diff&rev=348940&r1=348939&r2=348940
==============================================================================
--- branches/1.8/include/asterisk/pbx.h (original)
+++ branches/1.8/include/asterisk/pbx.h Thu Dec 22 20:09:16 2011
@@ -76,7 +76,10 @@
struct ast_sw;
/*! \brief Typedef for devicestate and hint callbacks */
-typedef int (*ast_state_cb_type)(char *context, char* id, enum ast_extension_states state, void *data);
+typedef int (*ast_state_cb_type)(char *context, char *id, enum ast_extension_states state, void *data);
+
+/*! \brief Typedef for devicestate and hint callback removal indication callback */
+typedef void (*ast_state_cb_destroy_type)(int id, void *data);
/*! \brief Data structure associated with a custom dialplan function */
struct ast_custom_function {
@@ -408,33 +411,54 @@
const char *ast_extension_state2str(int extension_state);
/*!
- * \brief Registers a state change callback
+ * \brief Registers a state change callback with destructor.
+ * \since 1.8.9
+ * \since 10.1.0
*
* \param context which context to look in
* \param exten which extension to get state
- * \param callback callback to call if state changed
+ * \param change_cb callback to call if state changed
+ * \param destroy_cb callback to call when registration destroyed.
* \param data to pass to callback
*
- * The callback is called if the state of an extension is changed.
+ * \note The change_cb is called if the state of an extension is changed.
+ *
+ * \note The destroy_cb is called when the registration is
+ * deleted so the registerer can release any associated
+ * resources.
*
* \retval -1 on failure
* \retval ID on success
*/
+int ast_extension_state_add_destroy(const char *context, const char *exten,
+ ast_state_cb_type change_cb, ast_state_cb_destroy_type destroy_cb, void *data);
+
+/*!
+ * \brief Registers a state change callback
+ *
+ * \param context which context to look in
+ * \param exten which extension to get state
+ * \param change_cb callback to call if state changed
+ * \param data to pass to callback
+ *
+ * \note The change_cb is called if the state of an extension is changed.
+ *
+ * \retval -1 on failure
+ * \retval ID on success
+ */
int ast_extension_state_add(const char *context, const char *exten,
- ast_state_cb_type callback, void *data);
+ ast_state_cb_type change_cb, void *data);
/*!
* \brief Deletes a registered state change callback by ID
*
- * \param id of the callback to delete
- * \param callback callback
- *
- * Removes the callback from list of callbacks
+ * \param id of the registered state callback to delete
+ * \param change_cb callback to call if state changed (Used if id == 0 (global))
*
* \retval 0 success
* \retval -1 failure
*/
-int ast_extension_state_del(int id, ast_state_cb_type callback);
+int ast_extension_state_del(int id, ast_state_cb_type change_cb);
/*!
* \brief If an extension hint exists, return non-zero
Modified: branches/1.8/main/pbx.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/pbx.c?view=diff&rev=348940&r1=348939&r2=348940
==============================================================================
--- branches/1.8/main/pbx.c (original)
+++ branches/1.8/main/pbx.c Thu Dec 22 20:09:16 2011
@@ -918,9 +918,14 @@
/*! \brief ast_state_cb: An extension state notify register item */
struct ast_state_cb {
+ /*! Watcher ID returned when registered. */
int id;
+ /*! Arbitrary data passed for callbacks. */
void *data;
- ast_state_cb_type callback;
+ /*! Callback when state changes. */
+ ast_state_cb_type change_cb;
+ /*! Callback when destroyed so any resources given by the registerer can be freed. */
+ ast_state_cb_destroy_type destroy_cb;
/*! \note Only used by ast_merge_contexts_and_delete */
AST_LIST_ENTRY(ast_state_cb) entry;
};
@@ -949,9 +954,9 @@
/* --- Hash tables of various objects --------*/
#ifdef LOW_MEMORY
-static const int HASH_EXTENHINT_SIZE = 17;
+#define HASH_EXTENHINT_SIZE 17
#else
-static const int HASH_EXTENHINT_SIZE = 563;
+#define HASH_EXTENHINT_SIZE 563
#endif
static const struct cfextension_states {
@@ -4465,27 +4470,16 @@
hint->laststate = state; /* record we saw the change */
/* For general callbacks */
- ao2_lock(statecbs);
cb_iter = ao2_iterator_init(statecbs, 0);
for (; (state_cb = ao2_iterator_next(&cb_iter)); ao2_ref(state_cb, -1)) {
- void *data;
-
- /*
- * Protect the data ptr because it could get updated by
- * ast_extension_state_add().
- */
- data = state_cb->data;
- ao2_unlock(statecbs);
- state_cb->callback(context_name, exten_name, state, data);
- ao2_lock(statecbs);
- }
- ao2_unlock(statecbs);
+ state_cb->change_cb(context_name, exten_name, state, state_cb->data);
+ }
ao2_iterator_destroy(&cb_iter);
/* For extension callbacks */
cb_iter = ao2_iterator_init(hint->callbacks, 0);
for (; (state_cb = ao2_iterator_next(&cb_iter)); ao2_ref(state_cb, -1)) {
- state_cb->callback(context_name, exten_name, state, state_cb->data);
+ state_cb->change_cb(context_name, exten_name, state, state_cb->data);
}
ao2_iterator_destroy(&cb_iter);
}
@@ -4497,9 +4491,26 @@
return 0;
}
-/*! \brief Add watcher for extension states */
-int ast_extension_state_add(const char *context, const char *exten,
- ast_state_cb_type callback, void *data)
+/*!
+ * \internal
+ * \brief Destroy the given state callback object.
+ *
+ * \param doomed State callback to destroy.
+ *
+ * \return Nothing
+ */
+static void destroy_state_cb(void *doomed)
+{
+ struct ast_state_cb *state_cb = doomed;
+
+ if (state_cb->destroy_cb) {
+ state_cb->destroy_cb(state_cb->id, state_cb->data);
+ }
+}
+
+/*! \brief Add watcher for extension states with destructor */
+int ast_extension_state_add_destroy(const char *context, const char *exten,
+ ast_state_cb_type change_cb, ast_state_cb_destroy_type destroy_cb, void *data)
{
struct ast_hint *hint;
struct ast_state_cb *state_cb;
@@ -4508,24 +4519,20 @@
/* If there's no context and extension: add callback to statecbs list */
if (!context && !exten) {
- /* Prevent multiple adds from adding the same callback at the same time. */
+ /* Prevent multiple adds from adding the same change_cb at the same time. */
ao2_lock(statecbs);
- state_cb = ao2_find(statecbs, callback, 0);
- if (state_cb) {
- state_cb->data = data;
- ao2_ref(state_cb, -1);
- ao2_unlock(statecbs);
- return 0;
- }
-
- /* Now insert the callback */
- if (!(state_cb = ao2_alloc(sizeof(*state_cb), NULL))) {
+ /* Remove any existing change_cb. */
+ ao2_find(statecbs, change_cb, OBJ_UNLINK | OBJ_NODATA);
+
+ /* Now insert the change_cb */
+ if (!(state_cb = ao2_alloc(sizeof(*state_cb), destroy_state_cb))) {
ao2_unlock(statecbs);
return -1;
}
state_cb->id = 0;
- state_cb->callback = callback;
+ state_cb->change_cb = change_cb;
+ state_cb->destroy_cb = destroy_cb;
state_cb->data = data;
ao2_link(statecbs, state_cb);
@@ -4566,14 +4573,18 @@
}
/* Now insert the callback in the callback list */
- if (!(state_cb = ao2_alloc(sizeof(*state_cb), NULL))) {
+ if (!(state_cb = ao2_alloc(sizeof(*state_cb), destroy_state_cb))) {
ao2_ref(hint, -1);
ao2_unlock(hints);
return -1;
}
- id = stateid++; /* Unique ID for this callback */
+ do {
+ id = stateid++; /* Unique ID for this callback */
+ /* Do not allow id to ever be -1 or 0. */
+ } while (id == -1 || id == 0);
state_cb->id = id;
- state_cb->callback = callback; /* Pointer to callback routine */
+ state_cb->change_cb = change_cb; /* Pointer to callback routine */
+ state_cb->destroy_cb = destroy_cb;
state_cb->data = data; /* Data for the callback */
ao2_link(hint->callbacks, state_cb);
@@ -4584,6 +4595,13 @@
return id;
}
+/*! \brief Add watcher for extension states */
+int ast_extension_state_add(const char *context, const char *exten,
+ ast_state_cb_type change_cb, void *data)
+{
+ return ast_extension_state_add_destroy(context, exten, change_cb, NULL, data);
+}
+
/*! \brief Remove a watcher from the callback list */
static int find_hint_by_cb_id(void *obj, void *arg, int flags)
{
@@ -4600,16 +4618,16 @@
}
/*! \brief ast_extension_state_del: Remove a watcher from the callback list */
-int ast_extension_state_del(int id, ast_state_cb_type callback)
-{
- struct ast_state_cb *p_cur = NULL;
+int ast_extension_state_del(int id, ast_state_cb_type change_cb)
+{
+ struct ast_state_cb *p_cur;
int ret = -1;
if (!id) { /* id == 0 is a callback without extension */
- if (!callback) {
+ if (!change_cb) {
return ret;
}
- p_cur = ao2_find(statecbs, callback, OBJ_UNLINK);
+ p_cur = ao2_find(statecbs, change_cb, OBJ_UNLINK);
if (p_cur) {
ret = 0;
ao2_ref(p_cur, -1);
@@ -4671,7 +4689,7 @@
while ((state_cb = ao2_callback(hint->callbacks, OBJ_UNLINK, NULL, NULL))) {
/* Notify with -1 and remove all callbacks */
/* NOTE: The casts will not be needed for v1.10 and later */
- state_cb->callback((char *) context_name, (char *) exten_name,
+ state_cb->change_cb((char *) context_name, (char *) exten_name,
AST_EXTENSION_DEACTIVATED, state_cb->data);
ao2_ref(state_cb, -1);
}
@@ -7229,7 +7247,7 @@
char data[1];
};
-AST_LIST_HEAD(store_hints, store_hint);
+AST_LIST_HEAD_NOLOCK(store_hints, store_hint);
static void context_merge_incls_swits_igps_other_registrars(struct ast_context *new, struct ast_context *old, const char *registrar)
{
@@ -7354,7 +7372,8 @@
struct ast_context *tmp;
struct ast_context *oldcontextslist;
struct ast_hashtab *oldtable;
- struct store_hints store = AST_LIST_HEAD_INIT_VALUE;
+ struct store_hints hints_stored = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
+ struct store_hints hints_removed = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
struct store_hint *saved_hint;
struct ast_hint *hint;
struct ast_exten *exten;
@@ -7424,7 +7443,7 @@
saved_hint->exten = saved_hint->data + strlen(saved_hint->context) + 1;
strcpy(saved_hint->exten, hint->exten->exten);
ao2_unlock(hint);
- AST_LIST_INSERT_HEAD(&store, saved_hint, list);
+ AST_LIST_INSERT_HEAD(&hints_stored, saved_hint, list);
}
}
@@ -7440,7 +7459,7 @@
* Restore the watchers for hints that can be found; notify
* those that cannot be restored.
*/
- while ((saved_hint = AST_LIST_REMOVE_HEAD(&store, list))) {
+ while ((saved_hint = AST_LIST_REMOVE_HEAD(&hints_stored, list))) {
struct pbx_find_info q = { .stacklen = 0 };
exten = pbx_find_extension(NULL, NULL, &q, saved_hint->context, saved_hint->exten,
@@ -7462,13 +7481,11 @@
/* Find the hint in the hints container */
hint = exten ? ao2_find(hints, exten, 0) : NULL;
if (!hint) {
- /* this hint has been removed, notify the watchers */
- while ((thiscb = AST_LIST_REMOVE_HEAD(&saved_hint->callbacks, entry))) {
- thiscb->callback(saved_hint->context, saved_hint->exten,
- AST_EXTENSION_REMOVED, thiscb->data);
- /* Ref that we added when putting into saved_hint->callbacks */
- ao2_ref(thiscb, -1);
- }
+ /*
+ * Notify watchers of this removed hint later when we aren't
+ * encumberd by so many locks.
+ */
+ AST_LIST_INSERT_HEAD(&hints_removed, saved_hint, list);
} else {
ao2_lock(hint);
while ((thiscb = AST_LIST_REMOVE_HEAD(&saved_hint->callbacks, entry))) {
@@ -7479,12 +7496,28 @@
hint->laststate = saved_hint->laststate;
ao2_unlock(hint);
ao2_ref(hint, -1);
- }
- ast_free(saved_hint);
+ ast_free(saved_hint);
+ }
}
ao2_unlock(hints);
ast_unlock_contexts();
+
+ /*
+ * Notify watchers of all removed hints with the same lock
+ * environment as handle_statechange().
+ */
+ while ((saved_hint = AST_LIST_REMOVE_HEAD(&hints_removed, list))) {
+ /* this hint has been removed, notify the watchers */
+ while ((thiscb = AST_LIST_REMOVE_HEAD(&saved_hint->callbacks, entry))) {
+ thiscb->change_cb(saved_hint->context, saved_hint->exten,
+ AST_EXTENSION_REMOVED, thiscb->data);
+ /* Ref that we added when putting into saved_hint->callbacks */
+ ao2_ref(thiscb, -1);
+ }
+ ast_free(saved_hint);
+ }
+
ast_mutex_unlock(&context_merge_lock);
endlocktime = ast_tvnow();
@@ -10630,15 +10663,15 @@
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;
+ ast_state_cb_type change_cb = arg;
+
+ return (state_cb->change_cb == change_cb) ? CMP_MATCH | CMP_STOP : 0;
}
int ast_pbx_init(void)
{
hints = ao2_container_alloc(HASH_EXTENHINT_SIZE, hint_hash, hint_cmp);
- statecbs = ao2_container_alloc(HASH_EXTENHINT_SIZE, NULL, statecbs_cmp);
+ statecbs = ao2_container_alloc(1, NULL, statecbs_cmp);
return (hints && statecbs) ? 0 : -1;
}
More information about the svn-commits
mailing list