<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1214/">https://reviewboard.asterisk.org/r/1214/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 11th, 2011, 2:42 p.m., <b>Jared Smith</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've admittedly been away from the Asterisk development community for a while, but I have to admit that I'm confused by the need for this particular patch.
The dialplan already has several capable tools for dealing with this sort of situation (using pattern matches, regular expression operators, CUT(), FIELDQTY(), and/or FILTER() just to name a few). Is there really a need to have this sort of thing done automagically by the extension matching code in Asterisk?
Yes, I can see how the pattern matching algorithm changed between 1.4 and later versions -- this was done very deliberately, and I personally think we're much better off because of it. (I could spend half a day talking about the intricacies of the pattern matching and how it's *so* much better now.) At the very least, I'd argue that if you're serious about adding this patch, that the principle of least surprise says you also add a flag (to either the [general] section of extensions.conf or to the [options] section of asterisk.conf) to enable the patch -- otherwise, you're going to catch creative folks like me off guard.
Besides -- I'm not sure the group of us can really enumerate all the different ways we might want to strip off part of the extension, or what characters we might want to split it on, or whether we want to somehow capture the pieces that have been split off. Today it's a semicolon, but tomorrow -- who knows? Instead of trying to hard-code one particular case, I'd feel much more comfortable making sure that the end users have the tools they need to do exactly what they want, and the knowledge on how to use said tools.</pre>
</blockquote>
<p>On May 11th, 2011, 2:52 p.m., <b>Leif Madsen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've talked to both Jonathan and Jared about this, and I think we're pretty much on the same page here. The main reason I'm not in favor of this change (and I wish I had realized what Jonathan was working on prior to today so I could have saved him the time) is that, as Jared alludes to, we have all the tools to deal with this change already. I would hate to over-engineer something that can be dealt with fairly trivially in the dialplan.
Lets say the example is we're getting something like 1234;phonecontext=incoming as the extension, and we only care about the 1234 part. Well we can simply use the CUT() function to get everything to the left of the semicolon. I believe this would work:
[incoming]
exten => _XXXX.,1,NoOp()
same => n,Set(RequestedExten=${CUT(EXTEN,\;,1)})
same => n,Goto(CallMangler,${RequestedExten},1)
[CallMangler]
exten => _XXXX,1,NoOp()
same => n,NoOp(Continue to be awesome)
Of course the dialplan could be made more elaborate to make sure we have a value to go to, that the extension exists, that the value is a number, and all sorts of things. All of this can already be done in the dialplan, and can be controlled by the administrator based on what they expect (and don't expect) their end points to send.</pre>
</blockquote>
<p>On May 11th, 2011, 2:59 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yeah, when it comes to finding extensions in the dialplan, you're likely right that people can work around this issue. The previous work Jonathan had done, that affects peer matching, absolutely needs to be done in the SIP code though, because there's no way for a user to work around that problem.</pre>
</blockquote>
<p>On May 11th, 2011, 4:25 p.m., <b>Leif Madsen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Kevin Fleming brought up a point about what Asterisk would respond to with my dialplan:
<kpfleming> jsmith: leifmadsen: it's been a long time since i worked on any of this stuff, so i may have forgotten... but if a SIP INVITE comes in for '1234;context=phone', hits the dialplan, which runs CUT() and goes a Goto(), and the resulting Goto() target does not exist, will that still result in a '404 Not Found' response to the INVITE?
The answer is that, with the dialplan I laid out, that a 603 Declined would be sent as the response, when you may wish for the response to actually be 404 Not Found.
Here is the dialplan snippet that you could use to check for the location in the dialplan first, and thus control the response to be what you might expect:
[incoming]
exten => _XXXX.,1,NoOp()
same => n,Set(Extension=${CUT(EXTEN,-,1)})
same => n,GotoIf($[${DIALPLAN_EXISTS(CallMangler,${Extension},1)]?CallMangler,${Extension},1)
same => n,Hangup(1)
[CallMangler]
exten => _XXXX,1,NoOp()
same => n,NoOp(Ohai)
Asterisk dialplan saves the day again :)
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@Mark: Pretty much every approach to fixing this that I've performed so far eventually boils down to finding the first semicolon in the user field and getting rid of it so that it can match more stuff. Reality of the matter is that using that approach, it's going to match on stuff that it should not legitimately be matching on.
For now, I'm going to propose an entirely different approach to this problem, but it's a rather significant one that I don't want to get started on it if it doesn't seem appropriate. Regardless, it seems to me like it would be very nice for the user to have some avenue for being able to match on extensions without having to rely on dialplan sorcery, regardless of how simple said dialplan sorcery appears to our dialplan adept.
Here's what I'm thinking:
Since there is generally an easily recognized pattern for any of these given cases that we want to strip on, let's leave it up to the user to decide what should be stripped. We can add another link list to the sip channel which is comprised entirely of stripper-strings (not referring to some especially lewd undergarments). Once we duplicate the string we are matching with into the uri, we can check to see if we have any of those words, iterate through them, and then search for those strings and if we find any of them, we can terminate them at the beginning of that string. Keep searching through the string until all the words are exhausted and then we should be left with a string that is stripped of any of the stripper-strings.
That keeps better in track with the spirit of customizability and should preserve the current behavior perfectly without a ton of added complication. As an added bonus, with a little extra work, we could even read which of those strings was in the uri and let the user see which ones were present using the channel function.
That said, it's a a rather complicated approach to a problem that arguably doesn't exist.</pre>
<br />
<p>- jrose</p>
<br />
<p>On May 11th, 2011, 9:46 a.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Russell Bryant, Mark Michelson, and David Vossel.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated 2011-05-11 09:46:56</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is sort of a natural follow-up to https://reviewboard.asterisk.org/r/1188/
Reading over RFC3261 and talking to mmichelson made the following things clear to me:
1. semi-colons are allowed in the user field of the uri. In fact, the very data I was trying to parse out is described as belonging in the user field by pages 156-158.
2. Theoretically, a user could make an extension something like 2005;bunch-of-uri-user-parameters
3. Since that's the case, simply parsing out this data before it ever gets used by anything based on semicolons could interfere with current behavior.
What I did to address it:
1. In the case of the user field in the uri from the bug...
"4254883646;phone-context=+1;npdi=yes" -- Asterisk needs to match 4254883646. Since this can't be safely removed in parsing, there is now a fail-triggered semi-colon delimited removal
performed in chan_sip's get_destination. If no extensions are made before it reaches the end, we check to see if there was a semicolon in the uri/decoded uri and if there was, we start the searches
over again with the uri terminated at the first semicolon.
2. If one of those uri semicolons is found, we preserve that data within a string field which can be accessed from the channel function.
3. A test was added to check that teluris that are converted into sip uris are read like it appears they should according to RFC3261.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I tested to make sure it didn't interfere with regular dialing behavior of sip phones by using one of my own sip phones.
I tested the basic behavior by forcing the following sip message using sipp:
INVITE sip:2005;phone-context=+1;npdi=yes@127.0.0.1:5060 SIP/2.0
Via: SIP/2.0/UDP 127.0.1.1:5062;branch=z9hG4bK-23024-1-0
From: "Lrrrr Schmrrr" <sip:sipp@127.0.1.1:5062>;tag=1
To: Asterisk <sip:2005:whoop;phone-context=+1;npdi=yes@127.0.0.1:5060>
Call-ID: 1-23024@127.0.1.1
CSeq: 1 OPTIONS
Contact: sip:sipp@127.0.1.1:5062
Max-Forwards: 70
Subject: Asterisk Testsuite
Content-Length: 0
And using this dialplan setup I'd check to make sure values were what I expected:
[sipp]
exten => 2005,1,Answer()
exten => 2005,n,Background(tt-weasels)
exten => 2005,n,NoOp(callerid => ${CALLERID(all)})
exten => 2005,n,NoOp(exten => ${EXTEN})
exten => 2005,n,NoOp(uri => ${CHANNEL(uri)})
exten => 2005,n,NoOp(user_options => ${CHANNEL(uri_user_parameters)})
exten => 2005,n,Wait(1)
exten => 2005,n,HangUp()
yielding these results:
-- Executing [2005@sipp:1] Answer("SIP/sipp-00000000", "") in new stack
-- Executing [2005@sipp:2] BackGround("SIP/sipp-00000000", "tt-weasels") in new stack
-- <SIP/sipp-00000000> Playing 'tt-weasels.gsm' (language 'en')
-- Executing [2005@sipp:3] NoOp("SIP/sipp-00000000", "callerid => "Lrrrr Schmrrr" <sipp>") in new stack
-- Executing [2005@sipp:4] NoOp("SIP/sipp-00000000", "exten => 2005") in new stack
-- Executing [2005@sipp:5] NoOp("SIP/sipp-00000000", "uri => sip:sipp@127.0.1.1:5062") in new stack
-- Executing [2005@sipp:6] NoOp("SIP/sipp-00000000", "user_options => phone-context=+1;npdi=yes") in new stack
-- Executing [2005@sipp:7] Wait("SIP/sipp-00000000", "1") in new stack
-- Executing [2005@sipp:8] Hangup("SIP/sipp-00000000", "") in new stack
I was paranoid that my goto statement would potentially cause a memory leak, so I also compared "memory show summary chan_sip.c" before and after the channel lived for both the case of the normal call and the case where it has to do the repetition with goto. I repeated the process with pbx.c and likewise saw no memory leak (though I thought I did at first when 4 allocations were made to pbx.c, but it turned out to be unrelated since I performed the test while creating hundreds of similar sip channels and saw no additional increases) Still, it's something worth looking at since I'm still rather likely to miss that sort of thing.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/view.php?id=18344">18344</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/channels/sip/dialplan_functions.c <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/channels/sip/include/sip.h <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/channels/sip/reqresp_parser.c <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/funcs/func_channel.c <span style="color: grey">(318337)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1214/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>