Review Board 1.7.16


Call Completion: libpri component

Review Request #522 - Created Feb. 19, 2010 and submitted

rmudgett
team/group/ccss
Reviewers
asterisk-dev
LibPRI
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/
A lot. :)

Unit tests were added to rosetest to test the encode/decode of the new ROSE call completion messages.
Posted (Feb. 25, 2010, noon)
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.  
  1. Thanks for the review.
/branches/1.4/pri_cc.c (Diff revision 1)
 
 
 
 
maybe this was intended. it just looked odd to me.  I saw this quite a bit.
  1. The indent utility likes to do this when the initial line is longer than the set length so I just left it.  It takes work to undo it.  If you run indent on the file again, it undoes your work.
/branches/1.4/pri_cc.c (Diff revision 1)
 
 
 
 
 
 
 
 
 
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.
  1. Two things:
    1) Initializing at declaration would be wasted by the early return.
    2) Initializing at the point of initial use makes as much sense.  The memset() in this case is part of filling in the structure with data to send in the facility message.
/branches/1.4/pri_facility.c (Diff revision 1)
 
 
 
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.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.