[asterisk-dev] [Code Review] Allow specifying which MixMonitor to stop

[Matt Jordan]

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



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9719>

    You have different names for the MixMonitor Id - here its "mixmonid", elsewhere its "MixMonitorID".  They should be consistent between the application and the Manager Action.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9720>

    This will truncate the pointer value, which on 64-bit machines means you aren't really getting the address of the mixmonitor struct.  This also means that you're no longer guaranteed to have a unique value - two MixMonitors could have the same lower 32-bit address values.
    
    Instead, just use the %p formatting option.  Again, don't set the width either (save for fitting with the length of the buffer)
    
    That does get into the fact that you don't know the length of the buffer in this method - you should check to make sure you don't overflow the buffer here or use snprintf.  The buffer should at least be able to handle a 64-bit address.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9723>

    A 9 character buffer would not be sufficient to store the address of a pointer on a 64-bit system; see previous.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9725>

    Is this no longer needed?



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9724>

    To keep the nesting lower, check for no datastore being returned from ast_channel_datastore_find, unlock and return -1.  Then you can unindent the rest of the code in this method.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9726>

    You should still compare it here  to "list" to see if they provided an invalid argument.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1643/#comment9721>

    Same finding as before.


- Matt


On Jan. 17, 2012, 5:26 a.m., telecos82 wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1643/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2012, 5:26 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Currently, if there are several MixMonitor started on a channel, there is no way to specify which MixMonitor to stop with StopMixMonitor. With this patch we will allow this. One limitation of current implementation was that mixmonitor datastore was not identified with a uid so there was no way to identify a concrete MixMonitor. Furthermore, when retrieving audiohook to dettach from channel, there was no control on which audiohook to dettach, it always got first audiohook found of type mixmonitor_spy_type. With this patch we are identifying MixMonitor datastore by the filename asociated to it, and dettaching the concrete audiohook contained on datastore. Furthermore a new CLI command "mixmonitor list <channel_name>" to help querying active mixmonitors on a channel.
> 
> 
> This addresses bug ASTERISK-19096.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19096
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c 348737 
> 
> Diff: https://reviewboard.asterisk.org/r/1643/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> telecos82
> 
>

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