<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/2227/">https://reviewboard.asterisk.org/r/2227/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 6th, 2012, 11 a.m. CET, <b>Matt Jordan</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;">So I have no idea why the svn merge didn't pull over the new files. I ended up just checking out earl-grey and looking over sip2cause.c.
Line 41, sip2causestruct definition - right now you have this structured as a list of sip2causestruct objects. This means to find a specific entry, you have to iterate over the entire list of sip2causestruct definitions in a table.
A possible faster way of structuring this data would be to use an ao2 object with the same definition of sip2causestruct, minus the 'next' pointer. You could then modify sip2causetable to contain a container of those objects:
struct sip2causetable {
struct ao2_container *table;
int default_code;
AST_DECLARE_STRING_FIELDS(
AST_STRING_FIELD(default_reason);
);
};
You would then create two different instances of sip2causetable - one would be for your SIP to ISDN mappings, and one would be for your ISDN to SIP mappings. They would differ in their key lookups - one would key off of sip2causestruct's sip field, the other would key off of sipcausestruct's cause field. This lets you do O(1) lookups of the appropriate mappings instead of O(n) lookups, which should dramatically speed up hangup_sip2cause and hangup_cause2sip.
As an aside, I don't see where defaultcode/defaultreason are used in sip2causetable. If they aren't needed, this simplifies things even more - you can get rid of the sip2causetable struct completely and replace it with two ao2_containers:
static struct ao2_container *sip2cause_lookups;
static struct ao2_container *cause2sip_lookups;
(As an aside, more use of the '_' please :-))</pre>
</blockquote>
<p>On December 6th, 2012, 11:05 a.m. CET, <b>Olle E Johansson</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 had an idea for the defaultcode, but seems to have forgotten it during the way forward. I'll review that again.
I don't think speed matters and will change significantly since there's only a handful of entries in the table. It's such a simple function and today we have a switch...
I will look for a container filled with underscores and apply it :-)
Thanks for the review. Seems like you agree with creating the new .c and .h files and the config file, which is good.</pre>
</blockquote>
<p>On December 6th, 2012, 12:33 p.m. CET, <b>Joshua Colp</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;">Presumably this has to be locked when iterated which could become a contention point on a system with many call teardowns so imo speed does indeed matter. Previously since it never changed it did not need to be protected.</pre>
</blockquote>
<p>On December 6th, 2012, 1:20 p.m. CET, <b>Olle E Johansson</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;">It can only change during a sip load/reload. During operations it doesn't change. We propably need a lock for the reload, but not for lookups. So there is no need for a lock during iteration - right?</pre>
</blockquote>
<p>On December 6th, 2012, 1:24 p.m. CET, <b>Joshua Colp</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;">You'd still have to lock it to make sure it doesn't change, but a read/write lock would work.</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;">How could it possibly change other than reload?</pre>
<br />
<p>- Olle E</p>
<br />
<p>On December 5th, 2012, 6:34 a.m. CET, Olle E Johansson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Olle E Johansson.</div>
<p style="color: grey;"><i>Updated Dec. 5, 2012, 6:34 a.m.</i></p>
<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/jira/browse/ASTERISK-20759">ASTERISK-20759</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">The SIP2CAUSE hangup code conversion tables has up to now been hard-coded in Asterisk. In some cases, like when building in-house ISDN/Q.SIG to SIP gateways, there's a need to manipulate this conversion.
With this code, advanced users can add a "private" conversion. This is added in front of the built-in conversions.
Asterisk conversion tables does not change in this patch. Everything should work as before. To shrink the chan_sip.c file a small bit I decided to move this functionality into a new source code file.
Adding:
- new source code file sip2cause.c and include file sip2cause.h
- new configuration file sip2cause.conf
Reviewboard doesn't seem accept the new files, so they have to be found in the branch itself.
http://svn.digium.com/svn/asterisk/team/oej/earl-grey-sip2cause-configurable-trunk
The new files are:
* http://svnview.digium.com/svn/asterisk/team/oej/earl-grey-sip2cause-configurable-trunk/configs/sip2cause.conf.sample
* http://svnview.digium.com/svn/asterisk/team/oej/earl-grey-sip2cause-configurable-trunk/channels/sip/sip2cause.c
* http://svnview.digium.com/svn/asterisk/team/oej/earl-grey-sip2cause-configurable-trunk/channels/sip/include/sip2cause.h
</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;">Tested all kinds of weird translations. This file should cause some errors (AST_CAUSE_SKREP doesn't exist, 903 is not a valid SIP reason code etc etc.
[sip2cause]
604 => AST_CAUSE_SKREP
404 => UNALLOCATED
599 Bad => USER_BUSY
486 => NORMAL_CLEARING
603 => UNALLOCATED
[cause2sip]
SKREP => 503 Service Failure
UNALLOCATED => 903 Go to hell
UNALLOCATED => 499 I don't want to do that.
USER_BUSY => 503 I am not feeling well</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/sip/include/sip_utils.h <span style="color: grey">(377205)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(377205)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2227/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>