[asterisk-commits] twilson: branch 1.8 r367292 - in /branches/1.8: include/asterisk/ main/

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Tue May 22 12:14:32 CDT 2012


Author: twilson
Date: Tue May 22 12:14:20 2012
New Revision: 367292

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=367292
Log:
Fix race condition for CEL LINKEDID_END event

This patch fixes to situations that could cause the CEL LINKEDID_END event to
be missed.

1) During a core stop gracefully, modules are unloaded when ast_active_channels
== 0. The LINKDEDID_END event fires during the channel destructor. This means
that occasionally, the cel_* module will be unloaded before the channel is
destroyed. It seemed generally useful to wait until the refcount of all
channels == 0 before unloading, so I added a channel counter and used it in the
shutdown code.

2) During a masquerade, ast_channel_change_linkedid is called. It calls
ast_cel_check_retire_linkedid which unrefs the linkedid in the linkedids
container in cel.c. It didn't ref the new linkedid. Now it does. 

Review: https://reviewboard.asterisk.org/r/1900/

Modified:
    branches/1.8/include/asterisk/cel.h
    branches/1.8/include/asterisk/channel.h
    branches/1.8/main/asterisk.c
    branches/1.8/main/cel.c
    branches/1.8/main/channel.c

Modified: branches/1.8/include/asterisk/cel.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/cel.h?view=diff&rev=367292&r1=367291&r2=367292
==============================================================================
--- branches/1.8/include/asterisk/cel.h (original)
+++ branches/1.8/include/asterisk/cel.h Tue May 22 12:14:20 2012
@@ -181,6 +181,15 @@
 void ast_cel_check_retire_linkedid(struct ast_channel *chan);
 
 /*!
+ * \brief Inform CEL that a new linkedid is being used
+ * \since 11
+ *
+ * \retval -1 error
+ * \retval 0 success
+ */
+int ast_cel_linkedid_ref(const char *linkedid);
+
+/*!
  * \brief Create a fake channel from data in a CEL event
  *
  * \note

Modified: branches/1.8/include/asterisk/channel.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/channel.h?view=diff&rev=367292&r1=367291&r2=367292
==============================================================================
--- branches/1.8/include/asterisk/channel.h (original)
+++ branches/1.8/include/asterisk/channel.h Tue May 22 12:14:20 2012
@@ -2124,8 +2124,11 @@
 /*! Cancels an existing shutdown and returns to normal operation */
 void ast_cancel_shutdown(void);
 
-/*! \return number of active/allocated channels */
+/*! \return number of channels available for lookup */
 int ast_active_channels(void);
+
+/*! \return the number of channels not yet destroyed */
+int ast_undestroyed_channels(void);
 
 /*! \return non-zero if Asterisk is being shut down */
 int ast_shutting_down(void);

Modified: branches/1.8/main/asterisk.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/asterisk.c?view=diff&rev=367292&r1=367291&r2=367292
==============================================================================
--- branches/1.8/main/asterisk.c (original)
+++ branches/1.8/main/asterisk.c Tue May 22 12:14:20 2012
@@ -1694,7 +1694,7 @@
 		for (;;) {
 			time(&e);
 			/* Wait up to 15 seconds for all channels to go away */
-			if ((e - s) > 15 || !ast_active_channels() || shuttingdown != niceness) {
+			if ((e - s) > 15 || !ast_undestroyed_channels() || shuttingdown != niceness) {
 				break;
 			}
 			/* Sleep 1/10 of a second */
@@ -1708,7 +1708,7 @@
 			ast_verbose("Waiting for inactivity to perform %s...\n", restart ? "restart" : "halt");
 		}
 		for (;;) {
-			if (!ast_active_channels() || shuttingdown != niceness) {
+			if (!ast_undestroyed_channels() || shuttingdown != niceness) {
 				break;
 			}
 			sleep(1);

Modified: branches/1.8/main/cel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/cel.c?view=diff&rev=367292&r1=367291&r2=367292
==============================================================================
--- branches/1.8/main/cel.c (original)
+++ branches/1.8/main/cel.c Tue May 22 12:14:20 2012
@@ -464,6 +464,31 @@
 	return tchan;
 }
 
+int ast_cel_linkedid_ref(const char *linkedid)
+{
+	char *lid;
+
+	if (ast_strlen_zero(linkedid)) {
+		ast_log(LOG_ERROR, "The linkedid should never be empty\n");
+		return -1;
+	}
+
+	if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) {
+		if (!(lid = ao2_alloc(strlen(linkedid) + 1, NULL))) {
+			return -1;
+		}
+		strcpy(lid, linkedid);
+		if (!ao2_link(linkedids, lid)) {
+			ao2_ref(lid, -1);
+			return -1;
+		}
+		/* Leave both the link and the alloc refs to show a count of 1 + the link */
+	}
+	/* If we've found, go ahead and keep the ref to increment count of how many channels
+	 * have this linkedid. We'll clean it up in check_retire */
+	return 0;
+}
+
 int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event_type,
 		const char *userdefevname, const char *extra, struct ast_channel *peer2)
 {
@@ -480,22 +505,10 @@
 	/* Record the linkedid of new channels if we are tracking LINKEDID_END even if we aren't
 	 * reporting on CHANNEL_START so we can track when to send LINKEDID_END */
 	if (cel_enabled && ast_cel_track_event(AST_CEL_LINKEDID_END) && event_type == AST_CEL_CHANNEL_START && chan->linkedid) {
-		char *lid;
-		if (!(lid = ao2_find(linkedids, (void *) chan->linkedid, OBJ_POINTER))) {
-			if (!(lid = ao2_alloc(strlen(chan->linkedid) + 1, NULL))) {
-				ast_mutex_unlock(&reload_lock);
-				return -1;
-			}
-			strcpy(lid, chan->linkedid);
-			if (!ao2_link(linkedids, lid)) {
-				ao2_ref(lid, -1);
-				ast_mutex_unlock(&reload_lock);
-				return -1;
-			}
-			/* Leave both the link and the alloc refs to show a count of 1 + the link */
-		}
-		/* If we've found, go ahead and keep the ref to increment count of how many channels
-		 * have this linkedid. We'll clean it up in check_retire */
+		if (ast_cel_linkedid_ref(chan->linkedid)) {
+			ast_mutex_unlock(&reload_lock);
+			return -1;
+		}
 	}
 
 	if (!cel_enabled || !ast_cel_track_event(event_type)) {

Modified: branches/1.8/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/channel.c?view=diff&rev=367292&r1=367291&r2=367292
==============================================================================
--- branches/1.8/main/channel.c (original)
+++ branches/1.8/main/channel.c Tue May 22 12:14:20 2012
@@ -93,6 +93,7 @@
 static int shutting_down;
 
 static int uniqueint;
+static int chancount;
 
 unsigned long global_fin, global_fout;
 
@@ -834,6 +835,11 @@
 	return channels ? ao2_container_count(channels) : 0;
 }
 
+int ast_undestroyed_channels(void)
+{
+	return ast_atomic_fetchadd_int(&chancount, 0);
+}
+
 /*! \brief Cancel a shutdown in progress */
 void ast_cancel_shutdown(void)
 {
@@ -1295,6 +1301,7 @@
 	ast_cdr_init(tmp->cdr, tmp);
 	ast_cdr_start(tmp->cdr);
 
+	ast_atomic_fetchadd_int(&chancount, +1);
 	ast_cel_report_event(tmp, AST_CEL_CHANNEL_START, NULL, NULL, NULL);
 
 	headp = &tmp->varshead;
@@ -2479,6 +2486,7 @@
 		 */
 		ast_devstate_changed_literal(AST_DEVICE_UNKNOWN, device_name);
 	}
+	ast_atomic_fetchadd_int(&chancount, -1);
 }
 
 /*! \brief Free a dummy channel structure */
@@ -6244,14 +6252,16 @@
  *  see if the channel's old linkedid is now being retired */
 static void ast_channel_change_linkedid(struct ast_channel *chan, const char *linkedid)
 {
+	ast_assert(linkedid != NULL);
 	/* if the linkedid for this channel is being changed from something, check... */
-	if (!ast_strlen_zero(chan->linkedid) && 0 != strcmp(chan->linkedid, linkedid)) {
-		ast_cel_check_retire_linkedid(chan);
-	}
-
+	if (!strcmp(chan->linkedid, linkedid)) {
+		return;
+	}
+
+	ast_cel_check_retire_linkedid(chan);
 	ast_string_field_set(chan, linkedid, linkedid);
-}
-
+	ast_cel_linkedid_ref(linkedid);
+}
 
 /*!
   \brief Propagate the oldest linkedid between associated channels




More information about the asterisk-commits mailing list