[asterisk-commits] russell: trunk r186928 - /trunk/channels/chan_sip.c

[SVN commits to the Asterisk project]

Author: russell
Date: Wed Apr  8 07:35:57 2009
New Revision: 186928

URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=186928
Log:
Update some comments and resolve potential memory corruption in chan_sip.

While browsing chan_sip the other day, I noticed this dangerous code in
dialog_needdestroy().  This function is an ao2_callback.  It is absolutely
_not_ okay to unlock the container from within this function.  It's also not
clear why it was useful.  Given that it could cause memory corruption, I have
removed it.

There was also a TODO comment left describing a potential implementation of
an improvement to the needdestroy handling.  I'm not convinced that what was
described is the best choice here, so I have briefly described the way that
this function is used today that could be improved.

Modified:
    trunk/channels/chan_sip.c

Modified: trunk/channels/chan_sip.c
URL: http://svn.digium.com/svn-view/asterisk/trunk/channels/chan_sip.c?view=diff&rev=186928&r1=186927&r2=186928
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Wed Apr  8 07:35:57 2009
@@ -14341,41 +14341,26 @@
 	}
 }
 
-/*! \brief this func is used with ao2_callback to unlink/delete all dialogs that
-   are marked needdestroy. It will return CMP_MATCH for candidates, and they
-   will be unlinked 
-
-   TODO: Implement a queue and instead of marking a dialog 
-   to be destroyed, toss it into the queue. Have a separate
-   thread do the locking and destruction */
+/*! 
+ * \brief Match dialogs that need to be destroyed
+ *
+ * \details This is used with ao2_callback to unlink/delete all dialogs that
+ * are marked needdestroy. It will return CMP_MATCH for candidates, and they
+ * will be unlinked.
+ *
+ * \todo Re-work this to improve efficiency.  Currently, this function is called
+ * on _every_ dialog after processing _every_ incoming SIP/UDP packet, or
+ * potentially even more often when the scheduler has entries to run.
+ */
 
 static int dialog_needdestroy(void *dialogobj, void *arg, int flags) 
 {
 	struct sip_pvt *dialog = dialogobj;
 	time_t *t = arg;
-	
-	/* log_show_lock(ao2_object_get_lockaddr(dialog)); this is an example of how to use log_show_lock() */
 
 	if (sip_pvt_trylock(dialog)) {
-		/* In very short-duration calls via sipp,
-		   this path gets executed fairly frequently (3% or so) even in low load
-		   situations; the routines we most commonly fight for a lock with:
-		   sip_answer (7 out of 9)
-		   sip_hangup (2 out of 9)
-		*/
-		ao2_unlock(dialogs);
-		usleep(1);
-		ao2_lock(dialogs);
-
-		/* I had previously returned CMP_STOP here; but changed it to return
-		   a zero instead, because there is no need to stop at the first sign
-		   of trouble. The old algorithm would restart in such circumstances,
-		   but there is no need for this now in astobj2-land. We'll
-		   just skip over this element this time, and catch it on the
-		   next traversal, which is about once a second or so. See the 
-		   ao2_callback call in do_monitor. We don't want the number of
-		   dialogs to back up too much between runs.
-		*/
+		/* Don't block the monitor thread.  This function is called often enough
+		 * that we can wait for the next time around. */
 		return 0;
 	}