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



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

    This section needs to be expanded in the case that the disconnect feature code is more than a single DTMF press.
    
    Actually, thinking about this a bit more, the digit collection should probably be done in wait_for_answer and the string of collected DTMF digits should be passed to this function. The return code of ast_feature_detect can be used to determine if the buffer for DTMF should be cleared or left as-is.



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

    There are spacing issues here, specifically with the AST_FEATURE_RETURN_HANGUP and AST_FEATURE_RETURN_SUCCESS definitions.



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

    This, and all other cases where ast_ have been re-added for functions which are not public, should be reverted.



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

    There is a potential for deadlock in this function. This function write-locks the feature_list and then read-locks the feature_groups.
    
    In features.c, in load_config, the the feature_groups list is write-locked and then the feature_list is read-locked.
    
    The potential for deadlocking is pretty slim here since it would require features.conf to be reloaded at exactly the precise moment that the feature is being detected here and features.conf would have to have parking lots defined in it as well. Still, I think reversing the order of the locks here would solve the issue.



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

    For sanity purposes, I'd probably add an ast_assert(result != NULL) before attempting to deference result.



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

    The way this function is written, you are undoing what I did to concatenate the chan and peer's DYNAMIC_FEATURES variables in rev 182029 of trunk. 


- Mark


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