[asterisk-dev] [Code Review] Allow disconnect feature before a call is bridged

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/195/#review575
-----------------------------------------------------------

Ship it!


Just fix this one problem and this will be ready to be merged. Great job, David!


/trunk/main/features.c
<http://reviewboard.digium.com/r/195/#comment1378>

    This is not one of the acceptable changes I presented in my last review. Take a closer look at the two possible suggestions I made :)


- Mark


On 2009-03-18 18:08:50, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/195/
> -----------------------------------------------------------
> 
> (Updated 2009-03-18 18:08:50)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> res_feature's main procedure is called within ast_bridge_call().  This means features such as the disconnect action are only available after the bridge is established.  The goal of this patch is to use the user configured feature code during the call setup phase.
> 
> This patch was created by a community member, sobomax, and originally assigned to someone else before being assigned to me.  I've taken what I believe was latest code and cleaned up what I could.  There were quite a few instances of string allocation taking place on the heap where they could have taken place on the stack.  I believe I caught them all, but that would be something to look for.  I also had some concerns about the way locking was done.  In detect_disconnect() the channel wasn't locked when I believe it should have been.  For a moment, features and the channel must be locked at the same time.  I don't suspect this to a problem, but it would be nice if someone could verify my locking order.  Other than that, my main concern is not fully understanding the implications of this code.  I understand what it does, but not everything it could affect. 
> 
> 
> This addresses bug 0011583.
>     http://bugs.digium.com/view.php?id=0011583
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_dial.c 183032 
>   /trunk/include/asterisk/features.h 183032 
>   /trunk/main/features.c 183032 
> 
> Diff: http://reviewboard.digium.com/r/195/diff
> 
> 
> Testing
> -------
> 
> I've tested the patch using the disconnect feature. Made a call and was able to successfully disconnect before bridging. 
> 
> 
> Thanks,
> 
> David
> 
>