[Home]

Summary:ASTERISK-13448: [patch] reg->username is parsed for each registration refresh rather than once on sip reload
Reporter:nick_lewis (nick_lewis)Labels:
Date Opened:2009-01-26 06:19:54.000-0600Date Closed:2010-05-26 14:46:51
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c.domainparse3.patch
( 1) chan_sip.c.patch
( 2) chan_sip.c-domparse.patch
( 3) chan_sip.c-domparse4.patch
( 4) chan_sip.c-domparse5.patch
( 5) nicklewispatch.diff
Description:The registration string can contain an expanded user portion of the form user@domain (and possibly user@domain:domainport). This expanded user portion is currently stored in reg->username and parsed each time there is a registration refresh. It would be better if the configuration was parsed only once and the sip_registry structure contained the atomic fields separately through the addition of, for example, reg->regdomain and reg->regdomainport elements
Comments:By: Olle Johansson (oej) 2009-01-26 06:39:52.000-0600

And the proposed patch is?
By: Olle Johansson (oej) 2009-01-26 06:40:55.000-0600

The user@domain:port will be removed btw. It's not the right place to add a port at all and should not have been committed.
By: nick_lewis (nick_lewis) 2009-01-26 08:05:18.000-0600

I know what you mean - a domain does not have a port. The problem is that the registrar does have a port. In the current implementation the domain info is used not only in the AOR (to/from headers) but also for the Registrar (request line) hence the need to have a "domainport"
By: Leif Madsen (lmadsen) 2009-01-26 08:09:49.000-0600

Nick:  note that feature requests can only be left open when there is a resulting code for it (it's a bug tracker policy).

If you would like to open discussion about some of these issues either prior to writing a patch, or in absence of one, then feel free to use the asterisk-dev mailing list where we can have a discussion on any issues you'd like to bring up.

(Think of it as pre-bug triage :))

Thanks for all your contributions lately!
By: Olle Johansson (oej) 2009-01-26 08:10:24.000-0600

So the provider requires you to register with one server and send calls to another?

That would be handled by two peers then, instead of confusing the syntax of the register=
By: nick_lewis (nick_lewis) 2009-01-26 08:43:21.000-0600

You will not be happy with the current 1.6 implementation then. If the domain is specified it is used not only for the AOR but also the request line. If I understand your "two peer" rule the request line should always be the host to which the registration is directly sent.

I will try to get you a patch once I have untangled it from all my other rejected patches
By: Olle Johansson (oej) 2009-01-26 08:45:31.000-0600

Exactly. I am not happy with 1.6 at all. It needs a lot of cleaning up to be something that can be supported again.
By: nick_lewis (nick_lewis) 2009-01-26 11:54:44.000-0600

Please find attached a patch that adds a reg->regdomain element without a reg->regdomainport element and makes registrar==host
By: Leif Madsen (lmadsen) 2009-01-26 13:43:33.000-0600

Moving to confirmed as there is now a patch attached. Thanks!
By: Olle Johansson (oej) 2009-01-26 13:47:33.000-0600

Starting a branch for a lot of registration fixes.
By: Leif Madsen (lmadsen) 2009-01-28 13:03:45.000-0600

oej: which branch did you start? I'd like to document it here.
By: nick_lewis (nick_lewis) 2009-01-29 10:32:12.000-0600

Is this patch ok as is?

Once this is applied it will be easier to get the cli to work with AORs (see issue 14248) because the AORs will always be ("%s@%s", reg->username, reg->regdomain) irrespective of the registrar host
By: Olle Johansson (oej) 2009-01-29 10:35:52.000-0600

Will check it as soon as I'm done with some other stuff.
By: Leif Madsen (lmadsen) 2009-05-12 19:50:55

Just pinging this issue as I'm going through issues that haven't been updated in a while.
By: nick_lewis (nick_lewis) 2009-05-13 03:58:28

I am happy to assist OEJ in any way I can to progress this and other sip registration issues. If OEJ does not feel that he has the time to deal with this area then I would be happy to assist any person to which the issues are reassigned
By: David Vossel (dvossel) 2009-07-14 10:44:21

Is their going to me any more work done with this?  If so, please continue your development in the trunk branch.  Since this is only a tweak and doesn't appear to fix anything that affects behavior, it should only be merged into trunk.
By: nick_lewis (nick_lewis) 2009-07-15 02:34:48

dvossel

Yes this was a minor tweak that slightly improved performance but not behavior.

I am not familiar with the way branches and merging operate. The patch was against head at the time that it was submitted. I am not sure whether it will apply cleanly now.
By: David Vossel (dvossel) 2009-07-16 11:18:38

The head is what we call trunk. (svn checkout http://svn.digium.com/svn/asterisk/trunk).  I've looked over the patch and it looks pretty good!  A few things have changed in transmit_register() involving string creation.  If you are willing to make the changes and bring it up to date for merging, I'll take a look at it again and see if we can't get it committed!  I'm fairly active on the tracker, so if you update the patch I should be able to review it quickly.  I won't let it sit around like it did last time.

Since this is tweak and not a bug fix/feature, if you don't have any plans to do additional work to the patch let me know so we can close the issue. Thanks for the contribution.
By: nick_lewis (nick_lewis) 2009-07-17 07:23:36

dvossel

Please find attached an up to date version of the patch for your review
By: David Vossel (dvossel) 2009-07-17 11:48:00

- /* If the registration username contains '@', then the domain should be used as
-   the equivalent of "fromdomain" for the registration */
- if (ast_strlen_zero(p->fromdomain)) {
- ast_string_field_set(p, fromdomain, fromdomain);
- }


The sip_pvt's fromdomain field used to be set before this, why was this taken out?  I don't fully understand the implications of this.


- /* Fromdomain is what we are registering to, regardless of actual
  host name from SRV */
+ /* Host is what we are registering to, regardless of domain */

This changes the way the addr field is built.  It used to be created using fromdomain, which was the user@domain:port part of the uri, if it was present.  Now it is built using the hostname only.  I haven't researched the importance of this, all I am doing is trying to verify no behavior changes are present in this patch.

Thanks for the update!
By: nick_lewis (nick_lewis) 2009-07-20 04:38:18

>The sip_pvt's fromdomain field used to be set before this
When before this point in the registration process was p->fromdomain set?

>why was this taken out?
Fromdomain was never relevant to sip registrations as the from-header must contain the same as the to-header. Previously p->fromdomain was used as a convenient but abused container for the dynamically parsed regdomain. Now that the regdomain has its proper place in the sip_registry this is not necessary

>This changes the way the addr field is built
Yes it may change the behaviour of the addr field (i.e. the request line). However I am confident that the behaviour in the patch is correct. If the asterisk registration string contains myself@sip.me.net@registrar.me.net then the from- and to-headers should contain myself@sip.me.net but the request line should contain registrar.me.net

I am sorry that I incorrectly described the patch as just a tweak. I had forgotten about this fix to the addr field. Do you want me to resubmit the patch to include the existing addr field bug and then submit a separate bug report with a patch to fix it?
By: David Vossel (dvossel) 2009-07-20 10:44:57

If the tweak was going in all the branches we could probably justify just throwing in the bug fix, but since the tweak is only going into trunk, and the bug fix needs to go into every branch, a separate report should be created.  So yes, go ahead and resubmit the patch with the existing addr field logic and submit a separate bug report for the other issue.
By: nick_lewis (nick_lewis) 2009-07-20 11:26:35

Ok please find attached a revised patch that retains the incorrect request line address
By: Digium Subversion (svnbot) 2009-07-20 15:45:29

Repository: asterisk
Revision: 207484

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r207484 | dvossel | 2009-07-20 15:45:28 -0500 (Mon, 20 Jul 2009) | 16 lines

reg->username is parsed only once on sip reload

The registration string can contain an expanded user portion of the
form user@domain. This expanded user portion was stored in
reg->username and parsed each time there is a registration refresh.
Now, the domain portion of the user is parsed and stored separately
in the regdomain field.

(closes issue ASTERISK-13448)
Reported by: Nick_Lewis
Patches:
     chan_sip.c.domainparse3.patch uploaded by Nick (license 657)
Tested by: Nick_Lewis, dvossel



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

http://svn.digium.com/view/asterisk?view=rev&revision=207484
By: Digium Subversion (svnbot) 2009-07-20 15:46:49

Repository: asterisk
Revision: 207486

_U  branches/1.6.2/

------------------------------------------------------------------------
r207486 | dvossel | 2009-07-20 15:46:49 -0500 (Mon, 20 Jul 2009) | 20 lines

Blocked revisions 207484 via svnmerge

........
 r207484 | dvossel | 2009-07-20 15:45:26 -0500 (Mon, 20 Jul 2009) | 16 lines
 
 reg->username is parsed only once on sip reload
 
 The registration string can contain an expanded user portion of the
 form user@domain. This expanded user portion was stored in
 reg->username and parsed each time there is a registration refresh.
 Now, the domain portion of the user is parsed and stored separately
 in the regdomain field.
 
 (closes issue ASTERISK-13448)
 Reported by: Nick_Lewis
 Patches:
       chan_sip.c.domainparse3.patch uploaded by Nick (license 657)
 Tested by: Nick_Lewis, dvossel
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=207486
By: nick_lewis (nick_lewis) 2009-08-13 03:08:07

dvossel

What is the status of this patch? I do not see it in trunk. (There is a note "Blocked revisions 207484 via svnmerge" that I guess may be relevant).

I may have misunderstood the process but I was waiting on this one to be implemented before generating a patch for issue ASTERISK-14501
By: Jason Parker (jparker) 2009-08-18 16:35:13

chan_sip.c.domainparse3.patch was committed to trunk in revision 207484 by dvossel.  Please proceed with ASTERISK-14501.
By: Digium Subversion (svnbot) 2009-12-15 12:43:08.000-0600

Repository: asterisk
Revision: 235132

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r235132 | dvossel | 2009-12-15 12:43:07 -0600 (Tue, 15 Dec 2009) | 14 lines

reverse minor sip registration regression

A registration regression caused by a code tweak in (issue ASTERISK-13448)
and a bug fix in (issue ASTERISK-14501) caused some sip registration
config entries to be constructed incorrectly.  Origially
issue ASTERISK-13448 contained the code tweak as well as a bug fix, but since
the issue was reported as a tweak the bug fix portion was moved into
issue ASTERISK-14501.  Both the tweak and the bug fix contained minor incorrect
logic that resulted in some SIP registrations to fail.

(issue ASTERISK-13448)
(issue ASTERISK-14501)


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

http://svn.digium.com/view/asterisk?view=rev&revision=235132
By: Digium Subversion (svnbot) 2009-12-15 12:44:18.000-0600

Repository: asterisk
Revision: 235133

_U  branches/1.6.2/

------------------------------------------------------------------------
r235133 | dvossel | 2009-12-15 12:44:18 -0600 (Tue, 15 Dec 2009) | 19 lines

Blocked revisions 235132 via svnmerge

........
 r235132 | dvossel | 2009-12-15 12:43:06 -0600 (Tue, 15 Dec 2009) | 14 lines
 
 reverse minor sip registration regression
 
 A registration regression caused by a code tweak in (issue ASTERISK-13448)
 and a bug fix in (issue ASTERISK-14501) caused some sip registration
 config entries to be constructed incorrectly.  Origially
 issue ASTERISK-13448 contained the code tweak as well as a bug fix, but since
 the issue was reported as a tweak the bug fix portion was moved into
 issue ASTERISK-14501.  Both the tweak and the bug fix contained minor incorrect
 logic that resulted in some SIP registrations to fail.
 
 (issue ASTERISK-13448)
 (issue ASTERISK-14501)
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=235133
By: Digium Subversion (svnbot) 2010-01-22 18:42:10.000-0600

Repository: asterisk
Revision: 242514

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_sip.c
U   branches/1.6.0/configs/sip.conf.sample

------------------------------------------------------------------------
r242514 | tilghman | 2010-01-22 18:42:09 -0600 (Fri, 22 Jan 2010) | 55 lines

Backporting register line parsing from trunk to fix a bad parsing error in 1.6.0.

(closes issue ASTERISK-15182)
Reported by: jamicque
Patches:
      20100114__issue16491.diff.txt uploaded by tilghman (license 14)
Tested by: jamicque

........
 r213098 | tilghman | 2009-08-19 16:05:17 -0500 (Wed, 19 Aug 2009) | 9 lines
 
 Better parsing for the "register" line
 Allows characters that are otherwise used as delimiters to be used within
 certain fields (like the secret).
 (closes issue ASTERISK-14045, closes issue ASTERISK-14612)
  Reported by: tilghman
  Patches:
        20090818__issue15008.diff.txt uploaded by tilghman (license 14)
  Tested by: lmadsen, tilghman
........
 r213635 | dvossel | 2009-08-21 16:02:50 -0500 (Fri, 21 Aug 2009) | 5 lines
 
 fixes sip register parsing when user@domain is used
 
 (issue ASTERISK-14045)
 (issue ASTERISK-14612)
........
 r215222 | tilghman | 2009-09-01 16:19:40 -0500 (Tue, 01 Sep 2009) | 3 lines
 
 Fix register such that lines with a transport string, but without an authuser, parse correctly.
 (AST-228)
........
 r215801 | tilghman | 2009-09-02 22:43:51 -0500 (Wed, 02 Sep 2009) | 5 lines
 
 Default the callback extension to "s".  This is a regression.
 (closes issue ASTERISK-14697)
  Reported by: elguero
  Change-type: bugfix
........
 r235132 | dvossel | 2009-12-15 12:43:06 -0600 (Tue, 15 Dec 2009) | 14 lines
 
 reverse minor sip registration regression
 
 A registration regression caused by a code tweak in (issue ASTERISK-13448)
 and a bug fix in (issue ASTERISK-14501) caused some sip registration
 config entries to be constructed incorrectly.  Origially
 issue ASTERISK-13448 contained the code tweak as well as a bug fix, but since
 the issue was reported as a tweak the bug fix portion was moved into
 issue ASTERISK-14501.  Both the tweak and the bug fix contained minor incorrect
 logic that resulted in some SIP registrations to fail.
 
 (issue ASTERISK-13448)
 (issue ASTERISK-14501)
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=242514
By: nick_lewis (nick_lewis) 2010-01-26 03:42:18.000-0600

From the comment in 0015539 it appears that the reported regression relates to that fix and not the tweak here. Therefore I will modify this tweak so that it does not to rely on 0015539 and so that it can be reapplied
By: nick_lewis (nick_lewis) 2010-01-26 04:15:14.000-0600

My mistake - this tweak does not depend on 0015539. What was the problem here. Is it with the port number?
By: David Vossel (dvossel) 2010-01-26 10:48:58.000-0600

I believe it had something to do with how the regdomain was set.  I remember looking at these two patches and knowing that I would have to modify this one in order to regain the expected behavior.

Here is an example of the config in our lab that broke.

-------------------------------
register => user@peer_1

[peer_1]
fromdomain=10.10.10.10
-------------------------------

the expected behavior for that register line is that peer_1 will be found, and the fromdomain will take its place as the address.
By: Olle Johansson (oej) 2010-01-26 10:57:51.000-0600

No, if you only have fromdomain in the peer, it's the domain in the From: header. The host should be the domain that replaces the "peer_1" and you have no host, David.
By: nick_lewis (nick_lewis) 2010-01-26 10:58:22.000-0600

Wow - I was unaware that the register string could (or should) suck fromdomain from the peer def like this. It may explain my previous question in this issue:

>>The sip_pvt's fromdomain field used to be set before this
>When before this point in the registration process was p->fromdomain set?
By: nick_lewis (nick_lewis) 2010-01-26 11:05:56.000-0600

oej our notes crossed

So the register string should be able to suck in the fromdomain from the peer def (without needing register=peer?etc...) but it should be used only in the from-header and not in the to-header and the request-uri. Have I understood correctly?
By: David Vossel (dvossel) 2010-01-26 11:20:51.000-0600

If r->hostname is peer_1, peer_1's host and fromdomain along with other peer data is copied into the dialog in create_addr_from_peer (which is called within create_addr() in transmit_register())

If the fromdomain is set for the dialog, that is used as the host addr in transmit_register.  I'm not saying whether this is correct or not, but this is behavior that has been around for quite some time.  It broke some configs, so I was asked to restore the previous behavior.
By: nick_lewis (nick_lewis) 2010-01-26 11:46:09.000-0600

Very many thanks for the explanation. I am sorry that I did not appreciate the special behaviour when r->hostname is not a real hostname but a peername. Can peernames have dots in them and if so which match takes priority - a DNS match or a peername match?

I will modify the patch for this issue to maintain the special peername behaviour.

Do other combinations need to be supported such as:
  register => user@peer_1@host.net
or:
  register => user@domain.net@peer_1
By: David Vossel (dvossel) 2010-04-14 11:31:21

Hey Nick, do you still plan on updating this issue?
By: nick_lewis (nick_lewis) 2010-04-19 09:06:19

Once I was back up to speed I was hoping to make use of the new parse_name_andor_addr() function but I guess I ought to knock this one off first
By: nick_lewis (nick_lewis) 2010-04-22 10:57:18

Oh hang on this breaks some of the regparse tests
By: nick_lewis (nick_lewis) 2010-04-23 08:59:47

This seems to work with the existing tests and a new one. It also seems to support the use of peers as the host
By: Digium Subversion (svnbot) 2010-05-26 14:46:49

Repository: asterisk
Revision: 266090

U   trunk/channels/chan_sip.c
U   trunk/channels/sip/config_parser.c
U   trunk/channels/sip/include/sip.h
U   trunk/main/app.c

------------------------------------------------------------------------
r266090 | dvossel | 2010-05-26 14:46:49 -0500 (Wed, 26 May 2010) | 20 lines

do all sip registry parsing before transmit_register

This patch breaks up every part of the sip registry string during
config parsing and removes all parsing from transmit_register().
Thanks to Nick_Lewis for contributing this patch!

(closes issue ASTERISK-13448)
Reported by: Nick_Lewis
Patches:
     chan_sip.c-domparse.patch uploaded by Nick Lewis (license 657)
     chan_sip.c.patch uploaded by Nick Lewis (license 657)
     chan_sip.c.domainparse3.patch uploaded by Nick Lewis (license 657)
     chan_sip.c-domparse4.patch uploaded by Nick Lewis (license 657)
     chan_sip.c-domparse5.patch uploaded by Nick Lewis (license 657)
     nicklewispatch.diff uploaded by dvossel (license 671)
Tested by: Nick_Lewis, dvossel

Review: https://reviewboard.asterisk.org/r/628/


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

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