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

David Vossel dvossel at digium.com
Mon Feb 22 17:47:34 CST 2010 

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


This is a result of a quick scan through the chan_sip changes.


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

    The ast_cc_service_type enum starts with AST_CC_NONE.  Would not having this in this map cause issues?  What would happen if sip_cc_service_map[0] was accessed for some reason later and this wasn't present?  The service string would not be initialized. 



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

    What happens here if HAVE_LIBXML2 isn't defined? Will this even compile? Its like doing
    
    struct event_state_compositor[] {
    /*nothing*/
    };
    
    *Update
    While I was reviewing this Mark and I discussed this and it will not cause a problem.  It will compile and ARRAY_LEN(event_state_compositor) will return 0.



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

    What if recall_monitor is NULL here?!  you check to verify it isn't in the block above this which indicates that it could be possible.



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

    Just a code style comment here: This function has a few things to keep up with on return. unref the agent, close pidf_doc, and free_text.  This looks like a good candidate to use a cleanup_failure label and a goto cleanup_failure instead of return -1.  Everything looks good here though. 


- David


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