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

Julian Lyndon-Smith asterisk at dotr.com
Mon Feb 8 12:31:20 CST 2010



> On 2010-02-07 07:55:45, Russell Bryant wrote:
> > trunk/apps/app_mixmonitor.c, lines 655-656
> > <https://reviewboard.asterisk.org/r/487/diff/9/?file=8023#file8023line655>
> >
> >     Should this be AMI_FAILURE or something?  There are a number of failure cases returning success.
> >     
> >     Also, I know this wasn't added by your patch, but since it's new, I wanted to comment on the naming of the return value constants.  I think that they should be AST_AMI_SUCCESS, etc., to match our current naming convention of always using an AST_ prefix.
> 
> Tilghman Lesher wrote:
>     A non-zero return value from AMI causes the session to abort.  In most cases, this is not what you want, which is why the constant is named AMI_DESTROY, not AMI_FAILURE.  In terms of the naming, there is precedent, in the constant AMI_VERSION.

Specifically asked that question, was told to use AMI_SUCCESS ;)

If we are going to change the constant names, will obviously change the code. I leave it to greater minds than mine !


> On 2010-02-07 07:55:45, Russell Bryant wrote:
> > trunk/apps/app_mixmonitor.c, line 675
> > <https://reviewboard.asterisk.org/r/487/diff/9/?file=8023#file8023line675>
> >
> >     You're leaking a channel reference.

thanks, fixed


> On 2010-02-07 07:55:45, Russell Bryant wrote:
> > trunk/include/asterisk/audiohook.h, line 287
> > <https://reviewboard.asterisk.org/r/487/diff/9/?file=8024#file8024line287>
> >
> >     Use:
> >     
> >     \retval 0 success
> >     \retval -1 failure

just confirming this, as lines 258 and 257 do the same thing. Is \retval a new way of documenting, and these references just older code ?


> On 2010-02-07 07:55:45, Russell Bryant wrote:
> > trunk/main/audiohook.c, lines 166-171
> > <https://reviewboard.asterisk.org/r/487/diff/9/?file=8025#file8025line166>
> >
> >     It's probably worth checking that the frame's datalen is non-zero before doing this.

would anything actually ever make it into this function if datalen was zero ?


- Julian


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


On 2010-02-06 08:20:29, Julian Lyndon-Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/487/
> -----------------------------------------------------------
> 
> (Updated 2010-02-06 08:20:29)
> 
> 
> 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
> 
>




More information about the asterisk-dev mailing list