[asterisk-commits] rizzo: branch rizzo/astobj2 r47253 - /team/rizzo/astobj2/channels/chan_sip.c

[asterisk-commits at lists.digium.com]

Author: rizzo
Date: Tue Nov  7 07:48:44 2006
New Revision: 47253

URL: http://svn.digium.com/view/asterisk?rev=47253&view=rev
Log:
put the deadlock avoidance code to lock a pvt and its owner channel
into a single function.
There is one suspicious usage, when called by the scheduler,
where we don't have the dialoglist locked. I am unclear whether
this is safe to do (because the object cannot be destroyed
by anyone else) or not.


Modified:
    team/rizzo/astobj2/channels/chan_sip.c

Modified: team/rizzo/astobj2/channels/chan_sip.c
URL: http://svn.digium.com/view/asterisk/team/rizzo/astobj2/channels/chan_sip.c?rev=47253&r1=47252&r2=47253&view=diff
==============================================================================
--- team/rizzo/astobj2/channels/chan_sip.c (original)
+++ team/rizzo/astobj2/channels/chan_sip.c Tue Nov  7 07:48:44 2006
@@ -867,7 +867,12 @@
 	enum referstatus status;			/*!< REFER status */
 };
 
-/*! \brief sip_pvt: PVT structures are used for each SIP dialog, ie. a call, a registration, a subscribe  */
+/*!
+ * PVT structures are used for each SIP dialog, ie. a call, a registration, a subscribe.
+ * Created and initialized by sip_alloc(), the descriptor goes into the list of
+ * descriptors (dialoglist), and there it stays i suppose forever
+ */
+
 struct sip_pvt {
 	ast_mutex_t pvt_lock;			/*!< Dialog private lock */
 	int method;				/*!< SIP method that opened this dialog */
@@ -1875,6 +1880,27 @@
 	return;
 }
 
+/*!
+ * Enter with a pvt locked, try to grab a lock on the owner channel too,
+ * with deadlock avoidance. Try as many times as specified (0 means forever),
+ * return 0 on success, -1 on failure.
+ * \note that we may temporarily drop the lock on pvt, so this is only
+ * safe to call if the pvt cannot disappear (i.e. dialoglist is locked,
+ * in the currently implementation).
+ */
+static int lock_pvt_and_owner(struct sip_pvt *pvt, int retries)
+{
+	while(pvt->owner && ast_channel_trylock(pvt->owner)) {
+		if (retries && retries-- <= 1)
+			return -1;	/* failed, too many loops */
+		sip_pvt_unlock(pvt);
+		usleep(1);
+		sip_pvt_lock(pvt);
+	}
+	ast_verbose("lock_pvt_and_owner %p %p success", pvt, pvt->owner);
+	return 0;
+}
+
 /*! \brief Retransmit SIP message if no answer (Called from scheduler) */
 static int retrans_pkt(void *data)
 {
@@ -1936,11 +1962,7 @@
 	pkt->retransid = -1;
 
 	if (ast_test_flag(pkt, FLAG_FATAL)) {
-		while(pkt->owner->owner && ast_channel_trylock(pkt->owner->owner)) {
-			sip_pvt_unlock(pkt->owner);	/* SIP_PVT, not channel */
-			usleep(1);
-			sip_pvt_lock(pkt->owner);
-		}
+		lock_pvt_and_owner(pkt->owner, 0 /* try forever */); /* XXX unsafe ? */
 		if (pkt->owner->owner) {
 			ast_set_flag(&pkt->owner->flags[0], SIP_ALREADYGONE);
 			ast_log(LOG_WARNING, "Hanging up call %s - no reply to our critical packet.\n", pkt->owner->callid);
@@ -8588,12 +8610,7 @@
 					ast_test_flag(&sip_pvt_ptr->flags[0], SIP_OUTGOING) ? "OUTGOING": "INCOMING",
 					sip_pvt_ptr->theirtag, sip_pvt_ptr->tag);
 
-			/* deadlock avoidance... */
-			while (sip_pvt_ptr->owner && ast_channel_trylock(sip_pvt_ptr->owner)) {
-				sip_pvt_unlock(sip_pvt_ptr);
-				usleep(1);
-				sip_pvt_lock(sip_pvt_ptr);
-			}
+			lock_pvt_and_owner(sip_pvt_ptr, 0 /* try forever */); /* safe, dialoglist is locked */
 			break;
 		}
 	}
@@ -14824,7 +14841,10 @@
 }
 
 
-/*! \brief helper function for the monitoring thread */
+/*!
+ * helper function for the monitoring thread.
+ * Called only in one place with dialoglist locked.
+ */
 static void check_rtp_timeout(struct sip_pvt *sip, time_t t)
 {
 	if (sip->rtp && sip->owner &&
@@ -14848,11 +14868,7 @@
 			     (t > sip->lastrtprx + sip->rtpholdtimeout))) {
 				/* Needs a hangup */
 				if (sip->rtptimeout) {
-					while (sip->owner && ast_channel_trylock(sip->owner)) {
-						sip_pvt_unlock(sip);
-						usleep(1);
-						sip_pvt_lock(sip);
-					}
+					lock_pvt_and_owner(sip, 0 /* forever */); /* safe, dialoglist is locked */
 					if (sip->owner) {
 						if (!(ast_rtp_get_bridged(sip->rtp))) {
 							ast_log(LOG_NOTICE,