[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/#review571
-----------------------------------------------------------


You're nearly there. Just a couple of small things left to fix.


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

    I think this if statement needs to be modified. I understand that if operation is defined, then it requires that chan, peer, and config are non-NULL. If that is the logic you are after here, then the if statement needs to be changed to the following:
    
    if (operation && (!peer || !chan || !config))
    
    or if you wanted to get all DeMorgan on me:
    
    if (operation && !(peer && chan && config))
    
    I think the first one is easier to follow though.



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

    (I actually already discussed this with David, but thought I'd make the discussion a bit more public)
    
    The deep copying used here is probably fine since feature interpretation doesn't need to be super optimized. David and I both recognize that doing this sort of copying as opposed to a shallow pointer copy also frees us from having to hold the features_list lock once we have found a matching feature.



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

    In order to be thread-safe, you need to make sure to make a local copy of the return of pbx_builtin_getvar_helper while chan is still locked. Look at what is done in feature_interpret for a good example. In fact, if you do what's done there, you can get rid of the dynamic_features_buf variable in this function.


- Mark


On 2009-03-18 15:27:15, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/195/
> -----------------------------------------------------------
> 
> (Updated 2009-03-18 15:27:15)
> 
> 
> 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 182800 
>   /trunk/include/asterisk/features.h 182800 
>   /trunk/main/features.c 182800 
> 
> 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
> 
>