[asterisk-dev] [Code Review] Add reporting of call time outs to app_dial, CDR

[Terry Wilson]

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



/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1608/#comment9175>

    I would just add AST_CAUSE_NO_USER_RESPONSE to the cases that increment num->nochan (CHANUNAVAIL) instead of adding a new TIMEDOUT status. Here are my issues with adding the new status:
    
    1) A protocol timeout is just a more specific description of a destination being unreachable, much like UNREGISTERED, or NO_ROUTE_DESTINATION which already map to CHANUNAVAIL. As far as I can tell, chan_iax2 and chan_sip are the only ones that actually would set a hangupcause as NO_USER_RESPONSE, so unless I am missing something (which is always possible) we would essentially be adding a DIALSTATUS that was specific for those channels.
    
    2) In my tests, dialing a non-listening address or an unreachable peer already returns CHANUNAVAIL and not NOANSWER as review states.
    
    3) We get a NOANSWER when the Dial timeout hits. Adding a state called TIMEDOUT for a protocol timeout when there is already a dial timeout is confusing. Of course, this is a naming issue and not really about whether the change should be made. But, when I tried coming up with better names...I kept running into things that reminded me of CHANUNAVAIL (UNREACHABLE, NORESPONSE, etc.).
    
    Looking at chan_dahdi, it looks like it maps AST_CAUSE_NO_ANSWER and AST_CAUSE_NO_USER_RESPONSE to the same R2 cause. So, there are at least some places that consider the two things equivalent--so there is that consistency issue to take into account too. In my short time looking at this, I can conclude that hangup causes are messy.


- Terry


On Dec. 2, 2011, 10:19 a.m., mjordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1608/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2011, 10:19 a.m.)
> 
> 
> Review request for Asterisk Developers, otherwiseguy and may213.
> 
> 
> Summary
> -------
> 
> This patch allows app_dial to report whether or not a channel timed out using the DIALSTATUS variable.  Previously, if a channel timed out (hangup cause code AST_CAUSE_NO_USER_RESPONSE), it would be treated as a "NOANSWER".  This patch adds a new value that can be returned in the DIALSTATUS variable "TIMEDOUT", and reports the value in CDR logs.  Note that the CEL already use the actual text of the hang up cause code, so modification to them is not necessary.
> 
> Note that adding a new DIALSTATUS hangup value appeared to require some modifications to the H.323 channel drivers - hence the updates for them included in this patch.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 346761 
>   /trunk/addons/chan_ooh323.c 346761 
>   /trunk/apps/app_dial.c 346761 
>   /trunk/channels/chan_h323.c 346761 
>   /trunk/channels/chan_sip.c 346761 
>   /trunk/include/asterisk/cdr.h 346761 
>   /trunk/main/cdr.c 346761 
> 
> Diff: https://reviewboard.asterisk.org/r/1608/diff
> 
> 
> Testing
> -------
> 
> Testing was done using two Polycom SIP phones.  After registration, one of the phones would be disconnected to force a timeout when Asterisk would dial it.  When the connected phone would dial the disconnected phone, the INVITE would time out and a DIALSTATUS of TIMEDOUT was returned in the dialplan.  Testing was also done to ensure that BUSY and CONGESTION were returned in normal scenarios.
> 
> Testsuite tests will be written for this patch, both to test the DIALSTATUS variable as well as the CDR logs.
> 
> 
> Thanks,
> 
> mjordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111202/e4bc1310/attachment.htm>