[asterisk-dev] [Code Review] Call Completion: Asterisk Component

Mark Michelson mmichelson at digium.com
Mon Feb 22 16:43:10 CST 2010 

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


Expect a new diff soon with these problems addressed, plus the files in tests/ to actually not be removed this time :)


/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3465>

    After seeing the result here on ReviewBoard, I think this comment block would probably serve better in sip.h instead of here. I'll make that change on the next upload.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3466>

    This struct can be made const.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3467>

    This struct can be made const.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3468>

    Another struct which could potentially go into sip.h



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3469>

    This comment is 100% false and needs to be removed.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/523/#comment3470>

    I need to do what this XXX comment says.
    
    The code works as-is because Asterisk always uses the proper xml namespace.


- Mark


On 2010-02-22 15:16:37, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/523/
> -----------------------------------------------------------
> 
> (Updated 2010-02-22 15:16:37)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is it folks. CCSS is at a point where a review will be beneficial.
> 
> CCSS stands for Call Completion Supplementary Services. An admittedly out-of-date
> overview of the architecture can be found in the file doc/CCSS_architecture.pdf
> in the CCSS branch. Off the top of my head, the big differences between what is
> implemented and what is in the document are as follows:
> 
> 1. We did not end up modifying the Hangup application at all.
> 2. The document states that a single call completion monitor may be used across
>    multiple calls to the same device. This proved to not be such a good idea
>    when implementing protocol-specific monitors, and so we ended up using one
>    monitor per-device per-call.
> 3. There are some configuration options which were conceived after the document
>    was written. These are documented in the ccss.conf.sample that is on this
>    review request.
>    
> For some basic understanding of terminology used throughout this code, see the
> ccss.tex document that is on this review.
> 
> This implements CCBS and CCNR in several flavors.
> 
> First up is a "generic" implementation, which can work over any channel technology
> provided that the channel technology can accurately report device state. Call
> completion is requested using the dialplan application CallCompletionRequest and can
> be canceled using CallCompletionCancel. Device state subscriptions are used in order
> to monitor the state of called parties.
> 
> Next, there is a SIP-specific implementation of call completion. This method uses the
> methods outlined in draft-ietf-bliss-call-completion-06 to implement call completion
> using SIP signaling. There are a few things to note here:
> 
> * The agent/monitor terminology used throughout Asterisk sometimes is the reverse of
>   what is defined in the referenced draft.
> 
> * Implementation of the draft required support for SIP PUBLISH. I attempted to write
>   this in a generic-enough fashion such that if someone were to want to write PUBLISH
>   support for other event packages, such as dialog-state or presence, most of the effort
>   would be in writing callbacks specific to the event package.
> 
> * A subportion of supporting PUBLISH reception was that we had to implement a PIDF
>   parser. The PIDF support added is a bit minimal. I first wrote a validation
>   routine to ensure that the PIDF document is formatted properly. The rest of the
>   PIDF reading is done in-line in the call-completion-specific PUBLISH-handling
>   code. In other words, while there is PIDF support here, it is not in any state
>   where it could easily be applied to other event packages as is.
> 
> Finally, there are a variety of ISDN-related call completion protocols supported. These
> were written by Richard Mudgett, and as such I can't really say much about their
> implementation. I'm leaving it to Richard to add any comments he wants about this
> matter. The libpri component of call completion support is posted as review 522
> on ReviewBoard.
> 
> The code added here is somewhat massive, so questions are welcome as much as critiques.
> Happy reviewing!
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 248346 
>   /trunk/apps/app_dial.c 248346 
>   /trunk/channels/chan_dahdi.c 248346 
>   /trunk/channels/chan_local.c 248346 
>   /trunk/channels/chan_sip.c 248347 
>   /trunk/channels/sig_analog.h 248346 
>   /trunk/channels/sig_analog.c 248346 
>   /trunk/channels/sig_pri.h 248346 
>   /trunk/channels/sig_pri.c 248346 
>   /trunk/channels/sip/include/sip.h 248346 
>   /trunk/configs/ccss.conf.sample PRE-CREATION 
>   /trunk/configs/chan_dahdi.conf.sample 248346 
>   /trunk/configure.ac 248346 
>   /trunk/doc/tex/asterisk.tex 248346 
>   /trunk/doc/tex/ccss.tex PRE-CREATION 
>   /trunk/funcs/func_callcompletion.c PRE-CREATION 
>   /trunk/include/asterisk/ccss.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 248346 
>   /trunk/include/asterisk/channelstate.h PRE-CREATION 
>   /trunk/include/asterisk/devicestate.h 248346 
>   /trunk/include/asterisk/frame.h 248346 
>   /trunk/include/asterisk/manager.h 248346 
>   /trunk/include/asterisk/rtp_engine.h 248346 
>   /trunk/include/asterisk/xml.h 248346 
>   /trunk/main/asterisk.c 248346 
>   /trunk/main/ccss.c PRE-CREATION 
>   /trunk/main/channel.c 248346 
>   /trunk/main/xml.c 248346 
>   /trunk/tests/test_amihooks.c 248346 
>   /trunk/tests/test_utils.c 248346 
> 
> Diff: https://reviewboard.asterisk.org/r/523/diff
> 
> 
> Testing
> -------
> 
> Too much to list :)
> 
> For a decent look at the tests executed, see the "Testing Done" section of review request 410.
> Those tests have all been run using combinations of generic, SIP, and ISDN agents and monitors.
> 
> 
> Thanks,
> 
> Mark
> 
>

More information about the asterisk-dev mailing list