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



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/195/#comment1315>

    I'm pretty sure you were going to do this anyway before merging, but these comments should be removed.



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/195/#comment1316>

    This one too.



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/195/#comment1319>

    Taking a look through the code, the feature_mask for dynamic features is always zeroed out. This for loop is useless as a result.



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/195/#comment1321>

    I did some testing of this patch and found that in order to make my SIP phone pass DTMF before the call was bridged, I had to change this if statement to the following:
    
    if (ast_test_flag64(&opts, OPT_DTMF_EXIT) || ast_test_flag64(&opts, OPT_CALLER_HANGUP)) {



/trunk/include/asterisk/features.h
<http://reviewboard.digium.com/r/195/#comment1317>

    This comment lists the locks in the incorrect order.



/trunk/include/asterisk/features.h
<http://reviewboard.digium.com/r/195/#comment1318>

    And so does this one.



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

    This function is nearly identical to feature_interpet. I think what would work best here is for ast_feature_detect to just call feature_interpret directly, with no other code in the function at all. Then, feature_interpret can be modified as necessary.
    
    I think there are only two necessary modifications for feature_interpret
    
    1. Make sure it doesn't crash when the peer pointer passed in is NULL.
    
    2. Change the function to take an ast_feature_interpret_result pointer and copy the logic used in the current ast_feature_detect into feature_interpret. I also think that feature_interpret should be able to function fine with a NULL ast_feature_interpret_result pointer passed in.


- Mark


On 2009-03-17 15:31:20, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/195/
> -----------------------------------------------------------
> 
> (Updated 2009-03-17 15:31:20)
> 
> 
> 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/main/features.c 182593 
>   /trunk/CHANGES 182607 
>   /trunk/apps/app_dial.c 182596 
>   /trunk/include/asterisk/features.h 182593 
> 
> 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
> 
>