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

asterisk-commits at lists.digium.com asterisk-commits at lists.digium.com
Wed Nov 22 04:55:46 MST 2006


Author: rizzo
Date: Wed Nov 22 05:55:45 2006
New Revision: 47925

URL: http://svn.digium.com/view/asterisk?view=rev&rev=47925
Log:
Start cleaning up the handling of outbound registrations.
In detail:

- Add some comment on the states sipregistrystate.
  This documentation is still largely incomplete;

- remove the 'expire' fields from struct sip_registry - now there
  is only one pending event, "timeout", which is used for both
  refresh and retransmissions. Much simpler to understand.

- set up a timeout also when retransmitting a REGISTER with auth set.
  Previously this was not done.

- cancel a timeout early in handle_response_register.
  I think this is always the thing to do. Not sure how does
  a registration restart on a permanent failure (or if we want it
  to happen at all).

- add a missing r->register_pvt = pvt_unref(r->register_pvt);
  in case "Failed to authenticate on REGISTER"

I still fail to understand why we are keeping the dialog alive
(sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT) instead of set_destroy(p) )
after receiving the 200 OK to the registration .

The handling of references to the sip_registry is still incorrect.


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?view=diff&rev=47925&r1=47924&r2=47925
==============================================================================
--- team/rizzo/astobj2/channels/chan_sip.c (original)
+++ team/rizzo/astobj2/channels/chan_sip.c Wed Nov 22 05:55:45 2006
@@ -1191,17 +1191,20 @@
 /*! \brief States for outbound registrations (with register= lines in sip.conf */
 enum sipregistrystate {
 	REG_STATE_UNREGISTERED = 0,	/*!< We are not registred */
-		/* Initial state. We should have r->expire > -1 to indicate
-		 * a scheduled transmission of the initial registration request.
-		 * When the timeout fires, we transmit the request and move to ...
+		/* Initial state. We should have a timeout scheduled for the initial
+		 * (or next) registration transmission, calling sip_reregister
 		 */
 	REG_STATE_REGSENT,	/*!< Registration request sent */
+		/* sent initial request, waiting for an ack or a timeout to
+		 * retransmit the initial request.
+		 */
 	REG_STATE_AUTHSENT,	/*!< We have tried to authenticate */
 	REG_STATE_REGISTERED,	/*!< Registred and done */
 	REG_STATE_REJECTED,	/*!< Registration rejected */
 	REG_STATE_TIMEOUT,	/*!< Registration timed out */
 	REG_STATE_NOAUTH,	/*!< We have no accepted credentials */
 	REG_STATE_FAILED,	/*!< Registration failed after several tries */
+		/* fatal - no chance to proceed */
 };
 
 /*!
@@ -1233,10 +1236,9 @@
 		AST_STRING_FIELD(random);
 	);
 	int portno;			/*!<  Optional port override */
-	int expire;			/*!< Sched ID of expiration */
 	int expiry;			/*!< Value to use for the Expires header */
 	int regattempts;		/*!< Number of attempts (since the last success) */
-	int timeout; 			/*!< sched id of sip_reg_timeout */
+	int timeout; 			/*!< sched id of next event */
 	int refresh;			/*!< How often to refresh */
 	struct sip_pvt *register_pvt;		/*!< create a sip_pvt structure for each outbound "registration dialog" in progress */
 	enum sipregistrystate regstate;	/*!< Registration state (see above) */
@@ -1726,8 +1728,6 @@
 			ast_log(LOG_DEBUG, "Destroying active SIP dialog for registry %s@%s\n", reg->username, reg->hostname);
 		reg->register_pvt = sip_destroy(reg->register_pvt);
 	}
-	if (reg->expire > -1)
-		ast_sched_del(sched, reg->expire);
 	if (reg->timeout > -1)
 		ast_sched_del(sched, reg->timeout);
 	ast_string_field_free_pools(reg);
@@ -3231,7 +3231,7 @@
 	if (n != 1) {
 		ast_verbose("__sip_destroy on object with %d refs\n", n);
 		ao2_bt();
-		*(char *)0 = 1;/* crash */
+		// *(char *)0 = 1;/* crash */
 	}
 }
 	pvt_unref(p);	/* automatically calls pvt_destructor on ao2*/
@@ -4790,7 +4790,6 @@
 	if (secret)
 		ast_string_field_set(reg, secret, secret);
 	reg->regstate = REG_STATE_UNREGISTERED;
-	reg->expire = -1;
 	reg->expiry = default_expiry;
 	reg->timeout =  -1;
 	reg->refresh = default_expiry;
@@ -7367,6 +7366,7 @@
 	if (!r)
 		return 0;
 
+	/* XXX why do we want to keep a dialog here ??? */
 	pvt = r->register_pvt;	/* XXX do we have a lock on it ? */
 	if (pvt && !ast_test_flag(&pvt->flags[0], SIP_NO_HISTORY))
 		append_history(pvt, "RegistryRenew", "Account: %s@%s", r->username, r->hostname);
@@ -7375,7 +7375,7 @@
 	if (sipdebug)
 		ast_log(LOG_NOTICE, "   -- Re-registration for  %s@%s\n", r->username, r->hostname);
 
-	r->expire = -1;
+	r->timeout = -1;
 	transmit_register(r, NULL, NULL);
 	unref_registry(r);
 	return 0;
@@ -7531,7 +7531,6 @@
 	}
 
 	/* set up a timeout */
-	if (auth == NULL)  {
 		if (r->timeout > -1) {
 			ast_log(LOG_WARNING, "Still have a registration timeout, #%d - deleting it\n", r->timeout);
 			ast_sched_del(sched, r->timeout);
@@ -7539,7 +7538,6 @@
 		r->timeout = ast_sched_add(sched, global_reg_timeout * 1000, sip_reg_timeout, r);
 		if (option_debug)
 			ast_log(LOG_DEBUG, "Scheduled a registration timeout for %s id  #%d \n", r->hostname, r->timeout);
-	}
 
 	if (strchr(r->username, '@')) {
 		snprintf(from, sizeof(from), "<sip:%s>;tag=%s", r->username, p->tag);
@@ -12326,7 +12324,7 @@
 /*! \brief Handle responses on REGISTER to services.
  * In principle, we should have p->registry != NULL,
  * and p->registry->register_pvt == p, otherwise there is
- * something bogus
+ * something bogus.
  */
 static int handle_response_register(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno)
 {
@@ -12342,12 +12340,31 @@
 		return 1;
 	}
 
+	/*
+	 * Not sure if it is the right thing to do, but probably it is ok
+	 * to cancel the timeout here, since we have already matched the
+	 * packet with the sip_registry, and so this is a valid entry
+	 * (how about sequence numbers ?).
+	 */
+	if (r->timeout == -1) {
+		ast_log(LOG_NOTICE, "Got %d without pending timeout for registry '%s@%s'\n",
+			resp, r->username, r->hostname);
+	} else {
+		ast_sched_del(sched, r->timeout);
+		r->timeout = -1;
+		/* XXX in the future, remember the reference to r */
+	}
+
 	switch (resp) {
 	case 401:	/* Unauthorized */
 	case 407:	/* Proxy auth */
-		/* retransmit with proper auth credentials */
+		/* Possibly retransmit with proper auth credentials.
+		 * This is the only case where it makes sense to keep the old
+		 * dialog. In all other cases, we don't need it anymore.
+		 */
 		if (p->authtries == MAX_AUTHTRIES || do_register_auth(p, req, resp)) {
 			ast_log(LOG_NOTICE, "Failed to authenticate on REGISTER to '%s@%s' (Tries %d)\n", r->username, r->hostname, p->authtries);
+			r->register_pvt = pvt_unref(r->register_pvt);	/* don't care anymore */
 			set_destroy(p);
 		}
 		break;
@@ -12361,8 +12378,6 @@
 		/* XXX how is this useful given we have no pending event anymore ??? */
 		if (global_regattempts_max)
 			r->regattempts = global_regattempts_max+1;
-		ast_sched_del(sched, r->timeout);
-		r->timeout = -1;
 		set_destroy(p);
 		r->register_pvt = pvt_unref(r->register_pvt);
 		break;
@@ -12370,8 +12385,6 @@
 	case 423:	/* Interval too brief */
 		r->expiry = atoi(get_header(req, "Min-Expires"));
 		ast_log(LOG_WARNING, "Got 423 Interval too brief for service %s@%s, minimum is %d seconds\n", r->username, r->hostname, r->expiry);
-		ast_sched_del(sched, r->timeout);
-		r->timeout = -1;
 		set_destroy(p);
 		r->register_pvt = pvt_unref(r->register_pvt);
 		if (r->expiry > max_expiry) {
@@ -12392,9 +12405,8 @@
 			r->regattempts = global_regattempts_max+1;
 		set_destroy(p);
 		r->register_pvt = pvt_unref(r->register_pvt);
-		ast_sched_del(sched, r->timeout);
-		r->timeout = -1;
 		break;
+
 	case 200:	/* 200 OK */
 		r->regstate = REG_STATE_REGISTERED;
 		r->regtime = time(NULL);		/* Reset time of last succesful registration */
@@ -12402,23 +12414,16 @@
 		r->regattempts = 0;
 		if (option_debug)
 			ast_log(LOG_DEBUG, "Registration successful\n");
-		if (r->timeout > -1) {
-			if (option_debug)
-				ast_log(LOG_DEBUG, "Cancelling timeout %d\n", r->timeout);
-			ast_sched_del(sched, r->timeout);
-		}
-		r->timeout=-1;
+
+		/* unlink the dialog from the registry */
 		r->register_pvt = pvt_unref(r->register_pvt);
 		p->registry = NULL;
-		/* Let this one hang around until we have all the responses */
+
+		/* Let this one hang around until we have all the responses (XXX what for ?)*/
 		sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
 		/* set_destroy(p);	*/
 
 		/* set us up for re-registering */
-		/* figure out how long we got registered for */
-		if (r->expire > -1)
-			ast_sched_del(sched, r->expire);
-		r->expire = -1;
 		/* according to section 6.13 of RFC, contact headers override
 		   expires headers, so check those first */
 		expires = 0;
@@ -12462,7 +12467,7 @@
 		r->refresh= (int) expires_ms / 1000;
 
 		/* Schedule re-registration before we expire */
-		r->expire=ast_sched_add(sched, expires_ms, sip_reregister, r); 
+		r->timeout=ast_sched_add(sched, expires_ms, sip_reregister, r); 
 		unref_registry(r);
 	}
 	return 1;
@@ -17403,14 +17408,14 @@
 	ms = regspacing;
 	ASTOBJ_CONTAINER_TRAVERSE(&regl, 1, do {
 		ASTOBJ_WRLOCK(iterator);
-		if (iterator->expire > -1)
-			ast_sched_del(sched, iterator->expire);
+		if (iterator->timeout > -1)
+			ast_sched_del(sched, iterator->timeout);
 		ms += regspacing;
 		/* grab a reference to make sure objects do not go away when
 		 * the timeout is canceled.
 		 */
 		ASTOBJ_REF(iterator);
-		iterator->expire = ast_sched_add(sched, ms, sip_reregister, iterator);
+		iterator->timeout = ast_sched_add(sched, ms, sip_reregister, iterator);
 		ASTOBJ_UNLOCK(iterator);
 	} while (0)
 	);



More information about the asterisk-commits mailing list