Summary: | ASTERISK-05683: [branch] sip_register() cleanup and implement "register => peername" command | ||
Reporter: | Luigi Rizzo (rizzo) | Labels: | |
Date Opened: | 2005-11-23 06:31:22.000-0600 | Date Closed: | 2006-10-06 10:44:08 |
Priority: | Major | Regression? | No |
Status: | Closed/Complete | Components: | Channels/chan_sip/Registration |
Versions: | Frequency of Occurrence | ||
Related Issues: | |||
Environment: | Attachments: | ( 0) sip.20051125.diff ( 1) sip.diff | |
Description: | the register => ... statement in sip.conf requires a lot of parameters that are usually also in the corresponding peer section of the config file. To avoid repeating the information, this patch implements a variant of the register statement, so you can write register => foo [foo] type=peer ... and grab the info directly from the relevant section. To implement this, i needed to move the processing of register statements after the processing of the peer section from sip.conf I think the patch can be further improved by providing a "register" option in the [peer] section, so that you don't need to provide a separate statement for that. Too bad i only thought of that while i was writing this bug report :) On passing, also cleanup the sip_register() function, by removing some useless operations in the original implementation e.g.: 3205 stringp=username; 3206 username = strsep(&stringp, ":"); 3207 if (username) { 3208 secret = strsep(&stringp, ":"); 3209 if (secret) 3210 authuser = strsep(&stringp, ":"); 3211 } 3212 stringp = hostname; 3213 hostname = strsep(&stringp, "/"); 3214 if (hostname) 3215 contact = strsep(&stringp, "/"); 3216 if (ast_strlen_zero(contact)) 3217 contact = "s"; 3218 stringp=hostname; 3219 hostname = strsep(&stringp, ":"); 3220 porta = strsep(&stringp, ":"); 3221 line 3207, the test always succeeds because shortly before we have insured that the initial value of username (hence stringp) is not NULL, and the strsep() in line 3206 returns the original argument; line 3215: the only effect of the statement is to set contact=stringp, because, judging from the syntax, we do not have other fields in "contact" so stripping the additional slash (if any) is pointless and undocumented; line 3220: as above, before the call stringp should only contain the "port" component, so stripping additional ":" etc is pointless and undocumented, and the statement should just become porta=stringp ****** ADDITIONAL INFORMATION ****** The entire user/peer thing is still a bit confusing to me, because, if i understand it correctly, the "peer" list is used for two different purposes: * to match INCOMING calls on the IP address alone (when the From: match on the "user" list fails); * to provide parameters and credentials for OUTGOING calls. I would think that the lists for incoming and outgoing calls should be separate, so the INCOMING call match should be done on "user" records alone ("type=friend" is fine if you want to put the same record in both lists). I don't think this requires a major change in the code. | ||
Comments: | By: Olle Johansson (oej) 2005-11-23 08:36:54.000-0600 As we're about to remove the user completely, we won't change that now. What we will introduce in 1.3 is matching incoming calls on peer by name. By: Olle Johansson (oej) 2005-11-23 08:39:34.000-0600 Check my patch in 3583 and see if you can integrate that. By: Luigi Rizzo (rizzo) 2005-11-23 09:50:11.000-0600 oej, i need a clarification: this patch (and basically yours in ASTERISK-3504) are completely backward-compatible config-file changes (and code simplifcations) for what we have in 1.2. If i understand well, 1.3 will have a completely different implementation. So trying to integrate ASTERISK-3504 and this patch would only make sense if we want this feature in 1.2 as well. I think it would be nice, but this seems somewhat in contrast with your note 94236 ? By: Luigi Rizzo (rizzo) 2005-11-23 09:59:30.000-0600 also in your patch ASTERISK-3504 sip_peer and sip_registry are cross linked. However sip_peer never references the sip_registry, so this pointer is probably unneeded; sip_registry only references sip_peer for CLI purposes. Considering the locking issues involved in having these cross pointers, are there other reasons to use them or we can remove them and just store the sip_peer name in the sip_registry entry ? By: Olle Johansson (oej) 2005-11-23 10:37:33.000-0600 We will change the current chan_sip in the 1.3dev version for 1.4 to complete ongoing work. The new chan_sip v3 won't be ready for 1.4 release, I would say that we are targeting 1.6 release with that work. So there is a reason to change chan_sip in 1.3 to make it more easy to use. I haven't looked at my old patch for a long time, so I can't answer questions right now - have to spend some time with both patches to remember what I was up to :-) By: Luigi Rizzo (rizzo) 2005-11-24 07:16:42.000-0600 pdated the patch according to the comments in ASTERISK-3504. Now, instead of the "register =>..." statements in the general section (which is still functional as before), with this patch you can put "register => anything_here_is_ignored" in the [peer] section and this is equivalent to the [general] register => username:secret@host/regexten Compared with the standard register format user[:secret[:authuser]]@host[:port][/contact] you cannot (yet) supply a different authuser or port number, and regexten acts as contact (the latter makes a lot of sense given that they should match). Implementing authuser and port as well is trivial, we just need to decide whether we want separate options in the [peer] section, or we want to put them on the register => ... line. Also i am bit confused on whether we really need all three of username authuser fromuser or perhaps fromuser can coincide with authuser ? By: Olle Johansson (oej) 2006-01-09 10:02:34.000-0600 username is a strange option that really should not be involved here. Will create branch for this patch and test it. By: Olle Johansson (oej) 2006-01-09 10:02:59.000-0600 And I know that "username" is involved even though it should not be ;-) By: Olle Johansson (oej) 2006-01-09 14:01:58.000-0600 New branch: http://svn.digium.com/svn/asterisk/team/oej/sipregister By: Olle Johansson (oej) 2006-01-28 15:21:57.000-0600 The "sipregister" branch is now updated with new code that changes the matching between a registration and a call, so that we don't have to rely on matching by IP any more and will support multiple registrations to the same host properly. By: Luigi Rizzo (rizzo) 2006-01-28 18:12:27.000-0600 oej, a few minor comments on the diff between trunk and your branch: + the flag SIP_PAGE2_PEER_REGISTER is only used in build_peer() to determine if a 'register' line has been seen. However this is already achieved by looking at the local variable register_lineno, so the flag and the associated manipulations are unnecessary and should be removed. + in sip_register(), the cast below is useless: reg->peer = (struct sip_peer * ) peer; /* Save SIP peer binding for incoming calls */ + in sip_register(), the comment /* split host[:port][/contact] */ should probably read /* split host[:port][/extension] */ By: Luigi Rizzo (rizzo) 2006-01-28 18:30:53.000-0600 also, with reference to your branch could you explain how the matching is different from the one in the trunk ? In particular, i see you send to the remote peer a 'randomcontact' with a magic prefix instead of the requested extension, and then remap the randomcontact back to the requested extension on incoming calls. I don't understand how this helps - it seems to be a NOP if you request different extensions (which i believe is normally the case, given that you do want to tell where incoming calls are coming from, and the remapping can be easily done in the dialplan). By: Olle Johansson (oej) 2006-01-30 00:46:45.000-0600 Rizzo, I am not done yet :-) If you look at the latest commit, there's still a few "todo's". By: Olle Johansson (oej) 2006-01-30 00:46:46.000-0600 Rizzo, I am not done yet :-) If you look at the latest commit, there's still a few "todo's". By: Olle Johansson (oej) 2006-01-30 00:48:48.000-0600 rizzo: If you today have two register= lines to the same server and two matching peers, like "luigis_account" and "olles_account" - incoming calls will always match the last peer, even if the call comes from luigi's account. With this change, calls will match the proper peer used for registration, not match on IP address at all. It's a huge improvement when you register for multiple accounts on the same server. By: Serge Vecher (serge-v) 2006-06-12 20:14:55 ooh, last updated in January... Olle, how can I help you test this...? By: Olle Johansson (oej) 2006-06-15 09:35:34 I have a bug I need to fix in this... By: Luigi Rizzo (rizzo) 2006-10-06 10:44:07 committed in simplified form to 44566. If there is interest can be merged to 1.4 as well, but at the moment i am uncertain about that. |