[asterisk-dev] [Code Review] Call Completion: Asterisk Component
rmudgett at digium.com
rmudgett at digium.com
Tue Feb 23 15:22:20 CST 2010
> On 2010-02-23 13:22:02, David Vossel wrote:
> > /trunk/channels/chan_dahdi.c, line 2843
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8234#file8234line2843>
> >
> > c++ style comments ahhhhh
Those comments are intended to show an optimization since they are not used in this function. They are used in other functions that need those components of the dialstring.
> On 2010-02-23 13:22:02, David Vossel wrote:
> > /trunk/channels/chan_dahdi.c, line 12648
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8234#file8234line12648>
> >
> > the end of this function could read
> > .
> > .
> > .
> > {
> > #endif /* defined(THRESHOLD_DEVSTATE_PLACEHOLDER) */
> > #endif /* defined(HAVE_PRI) */
> > return AST_DEVICE_UNKNOWN;
> > }
> >
> >
> > That would get rid of the having two return AST_DEVICE_UNKNOWN statements and the #else which makes it a little easier to read. But, this isn't all that important. I just got a little lost in all the #if defined statements in this function. I almost made a comment at the end that there was no return statement when HAVE_PRI was not defined, but this is not the case.
Conditionals are notorious for adding confusion to code. I will make the suggestion.
> On 2010-02-23 13:22:02, David Vossel wrote:
> > /trunk/channels/chan_dahdi.c, line 16083
> > <https://reviewboard.asterisk.org/r/523/diff/2/?file=8234#file8234line16083>
> >
> > Another code style comment.
> >
> > In the code that follows, there are 4 consecutive
> >
> > #if defined(HAVE_PRI)
> > #if defined(HAVE_PRI_CCSS)
> > .
> > .
> > .
> > #endif
> > #endif
> >
> > All of these could be wrapped up in a single
> >
> > #if defined(HAVE_PRI) && defined(HAVE_PRI_CCSS)
> > .
> > .
> > .
> > #endif
> >
> > Maybe this was intended to make sure the #if defined will follow the functions if they are ever moved or something. Just thought I'd point it out.
Those functions have their own conditionals around them so it is easier to determine if the code is included or not. If they were all conditionaled under one block you would have to search to find if the code was actually included in the compile.
Inertia is the reason the
#if defined(HAVE_PRI)
#if defined(HAVE_PRI_CCSS)
...
#endif /* defined(HAVE_PRI_CCSS) */
#endif /* defined(HAVE_PRI) */
is not formatted
#if defined(HAVE_PRI) && defined(HAVE_PRI_CCSS)
...
#endif /* defined(HAVE_PRI) && defined(HAVE_PRI_CCSS) */
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/523/#review1556
-----------------------------------------------------------
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