[asterisk-dev] [Code Review] 2620: bridge features: Make one touch Monitor and MixMonitor into bridge features

[Matt Jordan]

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



/trunk/CHANGES
<https://reviewboard.asterisk.org/r/2620/#comment17483>

    I don't think we should have this be a generic failure sound for all features.
    
    There is already xferfailsound and pickupfailsound. If those aren't specified, is featurefailsound used instead? Do those options override this particular option? If not, how do I configure a generic feature failure sound but not play a failure sound when pickup fails?
    
    Having different failure sounds for different features provides more functionality that having one master failure sound. Since we already have two failure sounds for different features, it would be more disruptive to introduce a generic failure sound than simply to provide failure sounds for one touch recording.



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17484>

    These function names don't follow standard nomenclature.
    
    (1) The use of "ast_" is inappropriate here - even if it is embedded in the function name. "ast_" is a namespace used to denote public functions. These aren't public functions, and adding "ast_" adds no value.
    
    (2) installable is not typical nomenclature used for callbacks. Typically, callbacks are registered and unregistered, and the callbacks are suffixed with _cb or _callback. I'd rename these to:
    
    start_mixmonitor_callback
    stop_mixmonitor_callback
    



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17494>

    This error message doesn't account for what just happened.
    
    The problem isn't just that the bridge isn't automon capable - it is technically capable of it, as the channel has the feature hook. The problem is there isn't anyone in the bridge to start recording on.
    
    "Cannot start AutoMonitor for %s - no peer in bridge" is more reflective of the actual condition.



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17495>

    Put this in a separate private function.



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17496>

    I'm with Richard - I wouldn't put the assignment here in the condition. This applies to obtaining all of the channel variables in this patch.
    
    Instead, you can:
    
    foo = pbx_builtin_getvar_helper(...)
    foo = ast_strdupa(S_OR(foo, ""));
    
    Or:
    
    foo = pbx_builtin_getvar_helper(...)
    if (!ast_strlen_zero(foo)) {
       foo = ast_strdupa(foo);
    }
    



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17497>

    These are strings, so you should be using ast_strlen_zero here.
    



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17498>

    This feels wrong. If we don't get one of these parameters off our channel, we immediately go to the peer, blow all of our values away, and use the peers?
    
    What if I don't want to specify a prefix? Now it goes to the peers and uses its format instead?
    
    This applies to MixMonitor as well.



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17499>

    Capitalize filename if it starts a new sentence



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17500>

    Put this in a separate private function



/trunk/include/asterisk/mixmonitor.h
<https://reviewboard.asterisk.org/r/2620/#comment17485>

    I wouldn't follow the voicemail function installation nomenclature here.
    
    You should define a virtual table structure that provides the function pointers. The installation function should install a virtual table, not the specific functions.
    
    That allows this to be extended if additional functions need to be added.
    



/trunk/include/asterisk/mixmonitor.h
<https://reviewboard.asterisk.org/r/2620/#comment17486>

    Nomenclature should be:
    
    [namespace]_[object]_[verb]
    
    These functions should be named:
    
    ast_mixmonitor_start
    ast_mixmonitor_stop



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17491>

    Don't prefix private objects with ast_



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17487>

    Use a scoped lock here



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17492>

    If functions have already been registered, log a WARNING or an ERROR



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17488>

    And use a scoped lock here



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17493>

    Previously you return -1 if you fail to obtain the lock, here you assert but continue.
    
    In general, we don't check the return code of the lock functions. If you fail to obtain the lock, you are probably in a rather bad state (do you have the lock? Do you not? Can you ever obtain it?) and can't do a lot regardless.
    
    What's more, the lock libraries have lots of asserts in them (as well as things that trigger ast_do_crash), so adding more stuff at a higher layer doesn't add much value.



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17489>

    Scoped lock



/trunk/main/mixmonitor.c
<https://reviewboard.asterisk.org/r/2620/#comment17490>

    Scoped lock


- Matt Jordan


On June 17, 2013, 7:08 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2620/
> -----------------------------------------------------------
> 
> (Updated June 17, 2013, 7:08 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: asterisk-21553
>     https://issues.asterisk.org/jira/browse/asterisk-21553
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds Monitor and MixMonitor as bridge features.  In order to avoid using PBX execution and autoservice, I added some additional API for MixMonitor provided through loadable function callbacks provided by app_mixmonitor.
> 
> In order to make One Touch MixMonitor behave more like its Monitor equivalent, I went ahead and added a couple channel variables for playing start and stop recording sounds (TOUCH_MIXMONITOR_MESSAGE_START and TOUCH_MIXMONITOR_MESSAGE_STOP)
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 391828 
>   /trunk/apps/app_mixmonitor.c 391828 
>   /trunk/bridges/bridge_builtin_features.c 391828 
>   /trunk/configs/features.conf.sample 391828 
>   /trunk/include/asterisk/features_config.h 391828 
>   /trunk/include/asterisk/mixmonitor.h PRE-CREATION 
>   /trunk/main/features_config.c 391828 
>   /trunk/main/mixmonitor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2620/diff/
> 
> 
> Testing
> -------
> 
> Tried both Monitor and MixMonitor with various channel variables set for filename/format and start/stop sounds. They appear to be working as one would expect.
> Also checked the behavior of MixMonitor with app_mixmonitor loaded and unloaded. As expected, it is rejected smoothly when MixMonitor is not loaded.
> 
> 
> Thanks,
> 
> jrose
> 
>

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