[asterisk-dev] [Code Review] Mute / Unmute manager commands for MixMonitor

[Julian Lyndon-Smith]

> On 2010-02-04 10:15:10, Sean Bright wrote:
> > trunk/apps/app_mixmonitor.c, line 655
> > <https://reviewboard.asterisk.org/r/487/diff/6/?file=8003#file8003line655>
> >
> >     "... Must be read, write, or both."

done


> On 2010-02-04 10:15:10, Sean Bright wrote:
> > trunk/apps/app_mixmonitor.c, line 656
> > <https://reviewboard.asterisk.org/r/487/diff/6/?file=8003#file8003line656>
> >
> >     The RESULT_* definitions are for CLI command handlers, not manager actions.  This should just be 'return 0.'  In fact, all of the return statements in this function should just return 0, regardless of success vs. failure.
> >
> 
> Tilghman Lesher wrote:
>     Incorrect, RESULT_* definitions are for routines that are explicitly NOT cli handlers.  Those have their own defines (CLI_{SUCCESS|FAILURE|SHOWUSAGE}).  He should be using the macro definition RESULT_SUCCESS, though, not 0.
> 
> Sean Bright wrote:
>     Incorrect, this is not how we implement manager commands today.  If he wants to use the AMI_SUCCESS and AMI_FAILURE definitions that you introduced after your erroneous commentary, I would be fine with that.

use new AMI_SUCCESS constant


> On 2010-02-04 10:15:10, Sean Bright wrote:
> > trunk/apps/app_mixmonitor.c, line 666
> > <https://reviewboard.asterisk.org/r/487/diff/6/?file=8003#file8003line666>
> >
> >     "... Must be read, write, or both."

done


> On 2010-02-04 10:15:10, Sean Bright wrote:
> > trunk/apps/app_mixmonitor.c, lines 673-679
> > <https://reviewboard.asterisk.org/r/487/diff/6/?file=8003#file8003line673>
> >
> >     Since you return from the 'if' block, you don't need this part to be inside an 'else'.

done


> On 2010-02-04 10:15:10, Sean Bright wrote:
> > trunk/include/asterisk/audiohook.h, line 67
> > <https://reviewboard.asterisk.org/r/487/diff/6/?file=8004#file8004line67>
> >
> >     Since you're using bits here, you don't need to reserve one for 'both' you can just use:
> >     
> >         AST_AUDIOHOOK_MUTE_READ | AST_AUDIOHOOK_MUTE_WRITE

done, thanks


- Julian


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


On 2010-02-10 03:38:06, Julian Lyndon-Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/487/
> -----------------------------------------------------------
> 
> (Updated 2010-02-10 03:38:06)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> For PCI-DSS compliance we are not allowed to record a credit card number in a MixMonitor file. However, we must record all conversations ....
> 
> I have added a new feature to audiohooks so that you can mute either read / write or both types of frames - this allows for MixMonitor to mute either side of the conversation without affecting the conversation itself.
> 
> MixMonitor now has two manager commands
> 
> 1) manager show command MuteMixMonitor
> Action: MuteMixMonitor
> Synopsis: Mute a channel in MixMonitor
> Privilege: <none>
> Description: Mutes a Mixmonitor Channel.
> Variables:
>   Channel: Channel to mute.
>   Direction: Which part to mute. read|write|both (from channel|to channel|both channels).
> 
> 2) manager show command unMuteMixMonitor
> Action: unMuteMixMonitor
> Synopsis: unMute a channel in MixMonitor
> Privilege: <none>
> Description: unMutes a Mixmonitor Channel.
> Variables:
>   Channel: Channel to unmute.
>   Direction: Which part to unmute. read|write|both (from channel|to channel|both channels).
> 
> 
> This addresses bug 16740.
>     https://issues.asterisk.org/view.php?id=16740
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_mixmonitor.c 244767 
>   trunk/include/asterisk/audiohook.h 244767 
>   trunk/main/audiohook.c 244767 
> 
> Diff: https://reviewboard.asterisk.org/r/487/diff
> 
> 
> Testing
> -------
> 
> Tested on 1.4 : Tested muting each direction, and both directions, and unmuting, and then listening to the mixmonitor file. The mutes and unmutes all seemed to be in the right place at the right time
> Tested on trunk : Tested muting each direction, and both directions, and unmuting, and then listening to the mixmonitor file. The mutes and unmutes all seemed to be in the right place at the right time
> 
> 
> Thanks,
> 
> Julian
> 
>