[asterisk-dev] [Code Review]: Manage translation table between SIP and ISDN hangup causes

[Olle E Johansson]

> On Dec. 6, 2012, 10 a.m., Matt Jordan wrote:
> > 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 :-))
> 
> Olle E Johansson wrote:
>     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.
> 
> jcolp wrote:
>     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.

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?


- Olle E


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2227/#review7498
-----------------------------------------------------------


On Dec. 5, 2012, 5:34 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2227/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2012, 5:34 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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
> 
> 
> This addresses bug ASTERISK-20759.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20759
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 377205 
>   /trunk/channels/sip/include/sip_utils.h 377205 
> 
> Diff: https://reviewboard.asterisk.org/r/2227/diff
> 
> 
> Testing
> -------
> 
> 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
> 
> 
> Thanks,
> 
> Olle E
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121206/431205ea/attachment.htm>