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

Mark Michelson mmichelson at digium.com
Wed Feb 24 15:31:26 CST 2010 


> On 2010-02-23 17:56:57, David Vossel wrote:
> > /trunk/main/ccss.c, line 847
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8257#file8257line847>
> >
> >     It isn't obvious to me where this memory is freed.

Good catch. The backend was meant to be freed in ast_cc_monitor_unregister. I properly removed the backend from the list but neglected to actually free it there.


> On 2010-02-23 17:56:57, David Vossel wrote:
> > /trunk/main/ccss.c, line 901
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8257#file8257line901>
> >
> >     It isn't obvious to me where this memory is freed either.

Same for this one. I meant to free this in ast_cc_agent_unregister but did not.


> On 2010-02-23 17:56:57, David Vossel wrote:
> > /trunk/main/ccss.c, lines 1300-1303
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8257#file8257line1300>
> >
> >     Is there any reason we should be checking to make sure ast_sched_thread_del succeeded before decrementing the monitor's ref?

Well, the idea in this function is to not call ast_sched_del at all if *sched_id is -1. I suppose that since ast_cc_available_timer_expire and cc_generic_monitor_cancel_available_timer run in separate threads, it is possible that there could be a race here. I will take you up on this suggestion.


> On 2010-02-23 17:56:57, David Vossel wrote:
> > /trunk/main/ccss.c, line 2353
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8257#file8257line2353>
> >
> >     should be be checking to make sure this succeeded before decrementing the agent's ref?

Yep, same thing here. I'll be sure to do that.


- Mark


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


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