[Home]

Summary:ASTERISK-03760: [patch] Improve REGISTER= handling
Reporter:Olle Johansson (oej)Labels:
Date Opened:2005-03-24 17:49:33.000-0600Date Closed:2008-01-15 15:37:06.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) newregister.txt
( 1) newregister0521.txt
Description:* Add a maximum attempts limit (settable in sip.conf)
* Adds a sipdebug=yes|no setting in sip.conf, so you can debug from load/reload
* Adds a lot of error handling for registrations, we're giving up on 404 not found, or auth errors.
* Adds a new registration state "gaveup"

Disclaimer on file

****** ADDITIONAL INFORMATION ******

Please test!
Comments:By: Olle Johansson (oej) 2005-03-24 17:50:33.000-0600

I am not ready with this, but want feedback from testers. Still debugging a lot of packets on my network, seeing weird things (even without this patch :-)
By: Olle Johansson (oej) 2005-03-31 10:40:16.000-0600

Wow, I am amazed by all the feedback. Will return as soon as I've processed all the input in this bug report :-)
By: Olle Johansson (oej) 2005-04-03 15:22:43

I think this is pretty useful.
By: Kevin P. Fleming (kpfleming) 2005-04-03 15:27:25

Yes, this could be useful, along with the work in ASTERISK-3903933. If you can combine your efforts with the work in that bug (which needs reimplementation anyway), we can get these changes into CVS.
By: Olle Johansson (oej) 2005-04-04 01:25:06

kevin, can we take it one step at a time? Agree that the other bug need reworking, I haven't got much time the coming weeks due to travelling. This patch has been running for a while.
By: Kevin P. Fleming (kpfleming) 2005-04-05 00:20:32

I haven't tried to apply this CVS yet, but I see a few things that concern me... they are:

if (res != len) {
- ast_log(LOG_WARNING, "sip_xmit of %p (len %d) to %s returned %d: %s\n", data, len, ast_inet_ntoa(iabuf, sizeof(iabuf), p->sa.sin_addr), res, strerror(errno));
+ ast_log(LOG_WARNING, "Error: sip_xmit of %p (len %d) to %s returned %d: %s\n", data, len, ast_inet_ntoa(iabuf, sizeof(iabuf), p->sa.sin_addr), res, strerror(errno));
}

There is no need to add an "Error" prefix; it's already LOG_WARNING. If this is a more serious problem, use a more serious LOG_* category :-)

@@ -1313,9 +1316,6 @@
if(ast_test_flag((&global_flags_page2), SIP_PAGE2_RTCACHEFRIENDS)) {
ast_copy_flags((&peer->flags_page2),(&global_flags_page2), SIP_PAGE2_RTAUTOCLEAR|SIP_PAGE2_RTCACHEFRIENDS);
if(ast_test_flag((&global_flags_page2), SIP_PAGE2_RTAUTOCLEAR)) {
- if (peer->expire > -1) {
- ast_sched_del(sched, peer->expire);
- }
peer->expire = ast_sched_add(sched, (global_rtautoclear) * 1000, expire_register, (void *)peer);
}
ASTOBJ_CONTAINER_LINK(&peerl,peer);

Are you absolutely 100% positive this is the right thing to do? I believe this was added recently to solve a deadlock problem, and it seems to be correct to leave it in.

@@ -4417,7 +4425,7 @@
if (!r)
return 0;

- ast_log(LOG_NOTICE, "   -- Registration for '%s@%s' timed out, trying again\n", r->username, r->hostname);
+ ast_log(LOG_NOTICE, "   -- Registration for '%s@%s' timed out, trying again (Try #%d)\n", r->username, r->hostname, r->regattempts);
if (r->call) {
/* Unlink us, destroy old call.  Locking is not relevent here because all this happens
  in the single SIP manager thread. */

The config option is called 'attempts', let's be consistent and use 'attempt' intead of 'try'.

@@ -4550,7 +4573,11 @@
snprintf(to, sizeof(to), "<sip:%s@%s>", r->username, p->tohost);
}

- snprintf(addr, sizeof(addr), "sip:%s", p->tohost);
+ //snprintf(addr, sizeof(addr), "sip:%s", p->tohost);
+ if (p->fromdomain && !ast_strlen_zero(p->fromdomain))
+ snprintf(addr, sizeof(addr), "sip:%s", p->fromdomain);
+ else
+ snprintf(addr, sizeof(addr), "sip:%s", r->hostname);
strncpy(p->uri, addr, sizeof(p->uri) - 1);

p->branch ^= rand();

Please, no C++ comments :-) Also, you've changed from using p->tohost to r->hostname here... I assume that is intentional.

handle_response_register() has a number of cases that are 'final' results to REGISTER requests, but they are handled differently, in subtle ways. If these differences are important, we better get comments in there to that effect.

Otherwise this looks like some nice work; I'm leery of committing it without some more testing response though.
By: Olle Johansson (oej) 2005-05-01 15:34:05

Will look at this shortly. This bug is waiting for an update from myself.
By: Kevin P. Fleming (kpfleming) 2005-05-01 18:55:25

I'm also working on reimplementing the "rtcache" stuff, there are just a number of things about it that are not very well done at the moment... I hope to have it done still this evening, so hopefully it won't interfere with what you are doing.
By: Olle Johansson (oej) 2005-05-02 01:25:27

This is for register=, not for inbound registrations for peers...
By: Olle Johansson (oej) 2005-05-21 04:24:37

Update to current CVS.
By: Olle Johansson (oej) 2005-06-03 15:34:15

kpfleming: This one has been laying around forever. I would really like this to move forward.
By: Kevin P. Fleming (kpfleming) 2005-06-03 18:12:38

Committed to CVS HEAD, thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:37:06.000-0600

Repository: asterisk
Revision: 5839

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r5839 | kpfleming | 2008-01-15 15:37:06 -0600 (Tue, 15 Jan 2008) | 2 lines

various improvements to outbound registrations and response handling (bug ASTERISK-3760)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5839