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

Mark Michelson mmichelson at digium.com
Mon Feb 22 12:11:59 CST 2010 

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


I gave myself a brief mini-review, mostly looking at the more glaring problems I noticed. In addition to the specific problems noted below, I also did notice the files which apparently were removed when they should not have been. In addition, I am aware of the more glaring whitespace issues present. I will correct both of these problems in the next uploaded diff.


/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/523/#comment3447>

    I suppose this comment should be more gender-neutral...



/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/523/#comment3448>

    This block of code should only execute if ignore_cc is not set.



/trunk/channels/chan_local.c
<https://reviewboard.asterisk.org/r/523/#comment3449>

    The setting of full_dest and the call to ast_cc_set_extension_dialstring are actually no longer necessary (however the call to ast_set_cc_interfaces_chanvar is important). For more information, see the comment made on the function ast_cc_set_extension_dialstring in include/asterisk/ccss.h on this review.



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

    I suppose this really should not be part of the call completion review, although it is a correct change to be making. I found that by defining REF_DEBUG where chan_sip suggested doing so, most ao2 calls were not logged in the /tmp/refs file. I suspect the reason is that one of the included files above also includes astobj2.h.
    
    I will make this code change in Asterisk trunk so that this change does not show up on this review request.



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

    These types should probably be moved to sip.h



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

    These structs could also stand to move to sip.h



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

    Another struct that could stand to move to sip.h



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

    Another struct that could be moved to sip.h



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

    This could use both some doxygen and a move to sip.h



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

    I have no idea why reviewboard has rendered this section yellow...



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

    This section is not new code. ReviewBoard is apparently on drugs.



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

    I should note here that the entity in the presence element of the PIDF body is just a peer name. This is a shortcut I took because the entity portion on an incoming call-completion PUBLISH is disregarded. For other event packages, this attribute can actually be important, so I will add a XXX note here indicating that this should be modified if other event packages want to use this PIDF construction function.



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

    I should probably put a big WARNING message here since something has gone badly wrong if this function gets called.



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

    This sort of comparison should probably be abstracted using some constants.



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

    And here, folks, we have an example of a XXX comment that I missed prior to uploading this code. This definitely needs to be fixed, and it's not that hard to do either.
    
    Although to be fair, the comment is accurate in that the purpose we supply does not actually influence behavior.



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

    It really wouldn't hurt to go ahead and enact this XXX comment.



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

    I find it hard to believe I actually wrote this comment.


- Mark


On 2010-02-19 16:10:01, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/523/
> -----------------------------------------------------------
> 
> (Updated 2010-02-19 16:10:01)
> 
> 
> 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 247295 
>   /trunk/apps/app_dial.c 247295 
>   /trunk/channels/chan_dahdi.c 247295 
>   /trunk/channels/chan_local.c 247295 
>   /trunk/channels/chan_sip.c 247295 
>   /trunk/channels/sig_analog.h 247295 
>   /trunk/channels/sig_analog.c 247295 
>   /trunk/channels/sig_pri.h 247295 
>   /trunk/channels/sig_pri.c 247295 
>   /trunk/channels/sip/include/sip.h 247295 
>   /trunk/configs/ccss.conf.sample PRE-CREATION 
>   /trunk/configs/chan_dahdi.conf.sample 247295 
>   /trunk/configure.ac 247295 
>   /trunk/doc/tex/asterisk.tex 247295 
>   /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 247295 
>   /trunk/include/asterisk/channelstate.h PRE-CREATION 
>   /trunk/include/asterisk/devicestate.h 247295 
>   /trunk/include/asterisk/frame.h 247295 
>   /trunk/include/asterisk/manager.h 247295 
>   /trunk/include/asterisk/rtp_engine.h 247295 
>   /trunk/include/asterisk/xml.h 247295 
>   /trunk/main/asterisk.c 247295 
>   /trunk/main/ccss.c PRE-CREATION 
>   /trunk/main/channel.c 247295 
>   /trunk/main/xml.c 247295 
>   /trunk/tests/test_amihooks.c 247295 
>   /trunk/tests/test_utils.c 247295 
> 
> 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