[Home]

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-0600Date Closed:2006-10-06 10:44:08
Priority:MajorRegression?No
Status:Closed/CompleteComponents: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.