Summary:	ASTERISK-13313: [patch] Setting registration expiry in registration string does not work
Reporter:	nick_lewis (nick_lewis)	Labels:
Date Opened:	2009-01-07 05:09:52.000-0600	Date Closed:	2009-02-20 17:54:02.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/Registration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) chan_sip.c-expiryrequest.patch ( 1) chan_sip.c-expiryrequest2.patch ( 2) chan_sip.c-expiryrequest3.patch ( 3) chan_sip.c-expiryrequest4.patch ( 4) chan_sip.c-expiryrequest5.patch ( 5) chan_sip.c-expiryrequest6.patch
Description:	In some applications it is necessary for an asterisk pbx to have very short registrations with a voip trunk provider if for example the asterisk pbx is behind a nat firewall and the provider does not provide other udp binding keep alive mechanisms. According to sip.conf the format for the registration string is [transport://]user[:secret[:authuser]]@host[:port][/contact][~expiry] so that it should be possible to set a shorter registration by appending a ~ and an expiry value e.g. ~60 for a one minute registration However the chan_sip.c code does not seem to make use of the [~expiry] portion so the default expiry value is always used for all registration strings.
Comments:	By: nick_lewis (nick_lewis) 2009-01-07 05:27:57.000-0600 Please find attached a patch that permits the expiry value specified in a registration string in sip.conf to be used by chan_sip.c when making registration requests to registrars. By: Leif Madsen (lmadsen) 2009-01-07 07:05:55.000-0600 Thanks for the submission! We'll be able to take a look at it once your license is approved! By: nick_lewis (nick_lewis) 2009-01-07 11:43:19.000-0600 Sorry there are a couple of problems with the patch 1. my name is splashed all over it 2. the [~expiry] parsing suffers the same problem as the [/contact] parsing in that it is mangled by the portnum retrieval in sip_parse_host function that is called just prior I will upload another patch that should resolve these problems and fix any ticket relating to the [/contact] parsing problem. By: nick_lewis (nick_lewis) 2009-01-07 11:47:04.000-0600 Please find revised patch attached. I will complete testing of it tomorrow By: nick_lewis (nick_lewis) 2009-01-08 03:27:12.000-0600 My testing completed ok By: Leif Madsen (lmadsen) 2009-01-08 07:34:35.000-0600 Thanks for the update Nick_Lewis. I have marked this issue as 'Confirmed' since there is now a licensed patched associated with it. Someone will get to it as soon as possible. Thanks! By: Leif Madsen (lmadsen) 2009-01-08 07:36:47.000-0600 One thing I see in the patch is a coding guideline issue: + if (expiryrequest) { + *expiryrequest++ = '\0'; + } I believe that should be: + if (expiryrequest) + *expiryrequest++ = '\0'; By: nick_lewis (nick_lewis) 2009-01-08 10:43:50.000-0600 Ok there was probably some ast_log stuff that I removed from here once it was working but forgot the braces. Can you tweak this yourself or do you want me to resubmit? By: Leif Madsen (lmadsen) 2009-01-08 14:12:00.000-0600 Would you mind resubmitting the patch with the changes? I don't quite have the time to patch a system, and create a diff, and upload it back here (my development system makes this not as trivial as I would like unfortunately). Thanks! By: nick_lewis (nick_lewis) 2009-01-09 03:21:19.000-0600 Sure - here it is (I know what you mean about development systems - mine has so many changes and so little tracking) By: Leif Madsen (lmadsen) 2009-01-09 07:25:26.000-0600 Thanks! By: Leif Madsen (lmadsen) 2009-01-09 07:32:34.000-0600 Assigned this to mnicholson as it should be a fairly straight forward patch to get implemented. By: nick_lewis (nick_lewis) 2009-01-12 05:59:10.000-0600 I have amended the patch so that it applies successfully to the current HEAD By: nick_lewis (nick_lewis) 2009-01-15 10:15:30.000-0600 Is this patch ok? I am happy to amend it as needed By: Matthew Nicholson (mnicholson) 2009-01-15 10:45:42.000-0600 I still need to review and test it.  I looked over it briefly, it looks ok so far. By: Leif Madsen (lmadsen) 2009-01-20 12:44:20.000-0600 Reassigning to otherwiseguy, which is just to nudge him to also give this a quick review. By: Terry Wilson (twilson) 2009-01-20 14:59:32.000-0600 Actually, the documentation for 1.6.0 does not mention the ~expiry option, only 1.6.1+ have that feature, which looks to be implemented in them.  I only mention this because the version listed in the bug is 1.6.0.  It doesn't look like the patch is actually against 1.6.0, though and applies cleanly (with some fuzz) to trunk so I'll test it against that. blitzrage lead you astray about the braces on the ifs, we require them around single line if statements now (too many bugs introduced without them).  It is a recent change, so forgive Leif for the confusion!  :-) reg->expiryrequest = default_expiry; if (expiryrequest)    reg->expiryrequest = atoi(expiryrequest); can be shortened to reg->expiryquest = expiryrequest ? atoi(expiryrequest) : default_expiry; also, if reg->expiryreqeust < 1 || < min_expiry, I would expect it to be set to min_expiry instead of default_expiry.  If it is > max_expiry, I would set it to max_expiry.  I would also use else if to chain those together since you can't have any of the conditions make any since at the same time (<1, < min_expiry, > max_expiry). Other than that, it looks good to me.  I haven't tested it yet, but it looks right. By: nick_lewis (nick_lewis) 2009-01-21 03:28:50.000-0600 I did not check whether the ~expiry option was in the documentation for 1.6.0 but it was in the log message giving the correct format of the registration string if a parsing error is encountered. The patch was originally against 1.6.0 but I redid it to work on HEAD as of the 2009-01-12. I have now looked at the original implementation of ~expiry in 1.6.1 and have some concerns with it. The main one is that it does not comply with rfc3261 as it lets the UAS determine the value of the Expire header in the REGISTER message after the first registration. The UAC should always be in control of this. That is why in my patch there is reg->expiryrequest separate from reg->expiry that always contains the configured value. Only reg->expiry is negotiated between UAC and UAS. Thanks for the clarification of the if braces and enhancements to the patch By: nick_lewis (nick_lewis) 2009-01-21 06:15:04.000-0600 I agree with setting to max_expiry and min_expiry if too big or small(or neg) but I think it is still necessary to warn the user when atoi(expiryrequest)==0 so that he can check his syntax rather than silently setting to default By: Terry Wilson (twilson) 2009-01-21 09:49:00.000-0600 Nick_Lewis: Sorry, I wasn't being very clear.  I wasn't meaning to remove any messages or combine the ifs, just talking about the, just change from setting to default to min.  Although an argument can be made that setting to < 1 is undefined behavior so it could be min, or max and instead just be set to default. By: nick_lewis (nick_lewis) 2009-01-21 10:49:54.000-0600 Ok here is another go at the patch with the discussed max and min settings. This patch is against 1.6.1 By: nick_lewis (nick_lewis) 2009-01-28 06:05:43.000-0600 Are there any further comments on this patch or is it ok to go ahead? Please note that if the fix for issue 14321 is applied first it will prevent this patch from applying because it contains a subset of this patch. By: nick_lewis (nick_lewis) 2009-01-28 07:54:57.000-0600 Please find updated patch that clears out some unused historic port parsing code. This patch is now a pure superset of the patch in issue 14321. Use this patch on its own to fix both 14321 and 14185 together. If you have fixed 14321 using the patch in that issue then you will need to undo that patch before you can fix this issue. By: Digium Subversion (svnbot) 2009-01-29 05:18:43.000-0600 Repository: asterisk Revision: 172234 U   trunk/channels/chan_sip.c ------------------------------------------------------------------------ r172234 | oej | 2009-01-29 05:18:43 -0600 (Thu, 29 Jan 2009) | 7 lines Make sure register= line supports both port and expiry at the same time. (closes issue ASTERISK-13313) Reported by: Nick_Lewis Patches:      chan_sip.c-expiryrequest6.patch uploaded by Nick (license 657) Tested by: Nick_Lewis ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=172234 By: Digium Subversion (svnbot) 2009-01-29 05:23:43.000-0600 Repository: asterisk Revision: 172235 _U  branches/1.6.1/ U   branches/1.6.1/channels/chan_sip.c ------------------------------------------------------------------------ r172235 | oej | 2009-01-29 05:23:43 -0600 (Thu, 29 Jan 2009) | 15 lines Merged revisions 172234 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r172234 | oej | 2009-01-29 12:19:29 +0100 (Tor, 29 Jan 2009) | 7 lines Make sure register= line supports both port and expiry at the same time. (closes issue ASTERISK-13313) Reported by: Nick_Lewis Patches:      chan_sip.c-expiryrequest6.patch uploaded by Nick (license 657) Tested by: Nick_Lewis ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=172235