[asterisk-dev] [Code Review] Don't miss control frames if a call is answered and hung up very quickly

[David Vossel]

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


I hate that we have to make a new Control frame type, but I feel strongly that this is the right solution at this point.  If it isn't a control frame, it has to be a new frame/subclass of some type.  Great Work!


/branches/1.4/include/asterisk/frame.h
<https://reviewboard.asterisk.org/r/740/#comment6021>

    If we make this new frame type, we need to document this heavily.  This frame type is very specific to ast_read. And should be queued no where else. It exists for a single purpose.  Maybe put AST_CONTROL_END_READ_XXX at the end so people will wonder why we called it that and read the comment. 



/branches/1.4/main/channel.c
<https://reviewboard.asterisk.org/r/740/#comment6019>

    You probably only want to queue this if it was the ast_check_hangup() case



/branches/1.4/main/channel.c
<https://reviewboard.asterisk.org/r/740/#comment6020>

    you could move this in the if above as an else case to checking for ast_check_hangup that needs to be done for queuing the frame



/branches/1.4/main/channel.c
<https://reviewboard.asterisk.org/r/740/#comment6018>

    i think we need an ast_frfree here before assigning f = NULL;


- David


On 2010-10-13 02:07:56, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/740/
> -----------------------------------------------------------
> 
> (Updated 2010-10-13 02:07:56)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and David Vossel.
> 
> 
> Summary
> -------
> 
> When an outgoing call is answered and hung up by the far end *very* quickly, we may not read any frames and therefor end up with a call that displays the wrong disposition/DIALSTATUS. The reason is because ast_queue_hangup() immediately sets the _softhangup flag on the channel and then queues the HANGUP control frame, but __ast_read refuses to read any frames if ast_check_hangup() indicates that a hangup request has been made (which it will if _softhangup is set). So, we end up losing control frames.
> 
> This change makes __ast_read continue to read frames even if a soft hangup has been requested. I believe this should be safe as we actually check farther down in __ast_read for HANGUP frames and return NULL just like we would have if we skipped the read via the goto done statement, and we don't call into any of the channel tech callbacks unless the readq is empty.
> 
> 
> This addresses bug 16946.
>     https://issues.asterisk.org/view.php?id=16946
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/frame.h 291108 
>   /branches/1.4/main/channel.c 291108 
> 
> Diff: https://reviewboard.asterisk.org/r/740/diff
> 
> 
> Testing
> -------
> 
> I have verified via sipp that when placing an outbound call that is answered and immediately hung up that the call shows up as answered (where it doesn't w/o the patch). I have also run the testsuite and verified that all tests continue to pass.
> 
> 
> Thanks,
> 
> Terry
> 
>