[asterisk-dev] [Code Review] Call Completion: libpri component

[David Vossel]

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


This code looks great!  Just from my initial scan over the diff nothing significant stood out to me, but I'm not familiar enough with libpri to make functionality comments here.  There were quite a few instances where c++ style comments were used to comment out blocks.  I'm not sure if you want to get rid of those or not.  Other than that I just had a few comments about things that make reviewing easier.  


/branches/1.4/pri_cc.c
<https://reviewboard.asterisk.org/r/522/#comment3578>

    maybe this was intended. it just looked odd to me.  I saw this quite a bit.



/branches/1.4/pri_cc.c
<https://reviewboard.asterisk.org/r/522/#comment3581>

    you could just do
    
    struct rose_msg_invoke msg = { 0, };
    
    and I think that initializes everything without having to do memset.  The only reason I'm pointing this out is that it makes the job of the reviewer much easier.  When I see a struct declared on the stack the first thing I look for is to see where it is initialized.  If it is just right there I don't have to hunt around for it.



/branches/1.4/pri_facility.c
<https://reviewboard.asterisk.org/r/522/#comment3588>

    If this has to be initialized then why not just initialize it up top at the declaration, same comment about it being easier to review.


- David


On 2010-02-19 16:51:38, rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/522/
> -----------------------------------------------------------
> 
> (Updated 2010-02-19 16:51:38)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Call Completion Supplementary Service (CCSS) added for the following switch types: ETSI PTMP, ETSI PTP, Q.SIG.
> Specifications:
> ETS 300 359 CCBS for PTMP and PTP
> ETS 301 065 CCNR for PTMP and PTP
> ECMA-186 Call Completion for Q.SIG
> 
> Several support services were added to support CC:
> Dummy Call Reference.
> Q.931 REGISTER message.
> Dynamic expansion of the number of available timers (up to 8192).
> Enhanced facility message handling.
> 
> Current implementation limitations preclude the following:
> CC service retention is not supported.
> Q.SIG path reservation is not supported.
> 
> The review for the asterisk component is located: https://reviewboard.asterisk.org/r/523/
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/Makefile 1499 
>   /branches/1.4/doc/cc_ptmp_agent.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptmp_agent_flattened.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptmp_monitor.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptmp_monitor_flattened.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptp_agent.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptp_agent_flattened.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptp_monitor.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_ptp_monitor_flattened.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_qsig_agent.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_qsig_agent_flattened.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_qsig_monitor.fsm PRE-CREATION 
>   /branches/1.4/doc/cc_qsig_monitor_flattened.fsm PRE-CREATION 
>   /branches/1.4/libpri.h 1499 
>   /branches/1.4/pri.c 1499 
>   /branches/1.4/pri_cc.c PRE-CREATION 
>   /branches/1.4/pri_facility.h 1499 
>   /branches/1.4/pri_facility.c 1499 
>   /branches/1.4/pri_internal.h 1499 
>   /branches/1.4/pri_q931.h 1499 
>   /branches/1.4/prisched.c 1499 
>   /branches/1.4/q931.c 1499 
>   /branches/1.4/rose.h 1499 
>   /branches/1.4/rose.c 1499 
>   /branches/1.4/rose_etsi_cc.c PRE-CREATION 
>   /branches/1.4/rose_internal.h 1499 
>   /branches/1.4/rose_qsig_cc.c PRE-CREATION 
>   /branches/1.4/rosetest.c 1499 
> 
> Diff: https://reviewboard.asterisk.org/r/522/diff
> 
> 
> Testing
> -------
> 
> A lot. :)
> 
> Unit tests were added to rosetest to test the encode/decode of the new ROSE call completion messages.
> 
> 
> Thanks,
> 
> rmudgett
> 
>