[asterisk-dev] [Code Review] Fix broken redirect out of MeetMe() as well as the deeper issue at hand

[Russell Bryant]

> On 2011-01-24 13:34:48, rmudgett wrote:
> > I don't see anything more to really comment about.

Thanks for the review!


> On 2011-01-24 13:34:48, rmudgett wrote:
> > /branches/1.8/main/features.c, line 734
> > <https://reviewboard.asterisk.org/r/1082/diff/3/?file=15278#file15278line734>
> >
> >     I don't know why this line was even here to begin with.  I don't think that _softhangup will ever be nonzero here.

Well, I think I'd like to leave it, and here's why:

1) If it's not needed, it's harmless (just a little bit of extra work in the middle of an already expensive operation (transfer)).

2) It is needed for some reason we can't think of, so instead of breaking it to find that out, we just leave it and let it keep working.  :-)


- Russell


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


On 2011-01-24 13:07:03, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1082/
> -----------------------------------------------------------
> 
> (Updated 2011-01-24 13:07:03)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Mantis issue #18585 reports that a channel redirect out of MeetMe() stopped working properly.  This issue includes a patch that resolves the issue by removing a call to ast_check_hangup() from app_meetme.c.  I left that in my patch, as it doesn't need to be there.  However, the rest of the patch fixes this problem with or without the change to app_meetme.
> 
> The key difference between what happens before and after this patch is the effect of the END_OF_Q control frame.  After END_OF_Q is hit in ast_read(), ast_read() will return NULL.  With the ast_check_hangup() removed, app_meetme sees this which causes it to exit as intended.  Checking ast_check_hangup() caused app_meetme to exit earlier in the process, and the target of the redirect saw the condition where ast_read() returned NULL.
> 
> Removing ast_check_hangup() works around the issue in app_meetme, but doesn't solve the issue if another application did the same thing.  There are also other edge cases where if an application finishes at the same time that a redirect happens, the target of the redirect will think that the channel hung up.  So, I made some changes in pbx.c to resolve it at a deeper level.  There are already places that unset the SOFTHANGUP_ASYNCGOTO flag in an attempt to abort the hangup process.  My patch extends this to remove the END_OF_Q frame from the channel's read queue, making the "abort hangup" more complete. 
> 
> 
> This addresses bug 18585.
>     https://issues.asterisk.org/view.php?id=18585
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_meetme.c 303508 
>   /branches/1.8/include/asterisk/channel.h 303508 
>   /branches/1.8/main/channel.c 303508 
>   /branches/1.8/main/features.c 303508 
>   /branches/1.8/main/pbx.c 303508 
> 
> Diff: https://reviewboard.asterisk.org/r/1082/diff
> 
> 
> Testing
> -------
> 
> I replicated the bug easily.  I verified that removing ast_check_hangup() fixes the issue and determined why it fixes it.  I also verified that the issue is resolved with the pbx.c changes, both with and without ast_check_hangup() still used in app_meetme.
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110124/3d7fa852/attachment-0001.htm>