> On Dec. 6, 2012, 11 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.
>
> Joshua Colp 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.
>
> Olle E Johansson wrote:
> 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?
>
> Joshua Colp wrote:
> You'd still have to lock it to make sure it doesn't change, but a
> read/write lock would work.
How could it possibly change other than reload?
- Olle E
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2227/#review7498
-----------------------------------------------------------
On Dec. 5, 2012, 6: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, 6:34 a.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-20759
> https://issues.asterisk.org/jira/browse/ASTERISK-20759
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> 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
>
>
> Diffs
> -----
>
> /trunk/channels/sip/include/sip_utils.h 377205
> /trunk/channels/chan_sip.c 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 Johansson
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
