[asterisk-dev] [Code Review] 2973: Add equivalent AMI actions for the bridge CLI commands.

[Scott Griepentrog]

> On Oct. 30, 2013, 2:44 p.m., rmudgett wrote:
> > /branches/12/main/bridge.c, lines 5057-5060
> > <https://reviewboard.asterisk.org/r/2973/diff/1/?file=47761#file47761line5057>
> >
> >     I don't think you need to prefix these headers with Bridge.  The only exception could be BridgeTechnology.

I initially had left out the Bridge prefix, but put it back in to match the output of BridgeInfo, which uses BridgeTechnology and BridgeType.  I could remove prefix from BridgePriority and BridgeSuspended, but it would then seem unbalanced.  Although redundant I think this is the better option. 


> On Oct. 30, 2013, 2:44 p.m., rmudgett wrote:
> > /branches/12/main/manager_bridges.c, line 132
> > <https://reviewboard.asterisk.org/r/2973/diff/1/?file=47762#file47762line132>
> >
> >     The bridge is not necessarily destroyed.

same as above issue.  Bridge is destroyed if specific channel not supplied.


> On Oct. 30, 2013, 2:44 p.m., rmudgett wrote:
> > /branches/12/main/bridge.c, line 4950
> > <https://reviewboard.asterisk.org/r/2973/diff/1/?file=47761#file47761line4950>
> >
> >     Manager action functions must not return non-zero.  Returning non-zero causes the AMI connection to be disconnected.

Pre-existing functions such as manager_bridge_info in manager_bridges.c return -1 on cases of missing arguments.  I have presumed these should be changed to return 0 as well.  Internal errors on memory allocation have been left to return -1.


> On Oct. 30, 2013, 2:44 p.m., rmudgett wrote:
> > /branches/12/main/manager_bridges.c, lines 127-129
> > <https://reviewboard.asterisk.org/r/2973/diff/1/?file=47762#file47762line127>
> >
> >     This should be required also.  I don't think this action should do a kick all.  If you want a kick all use BridgeDestroy instead.

Per ASTERISK-22356 requirements from Matt the kick is to perform bridge delete if channel omitted. Yes, it does appear redundant with Destroy.  But it does make parameter optional.

Additionally, in this update I'm implementing the original request from Matt that the bridge id does not have to be specified to kick the channel.  That also makes bridgeid optional, although one of bridgeid or channel has to be present.


- Scott


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


On Oct. 29, 2013, 4:47 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2973/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2013, 4:47 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22356
>     https://issues.asterisk.org/jira/browse/ASTERISK-22356
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds the following AMI events mirroring their CLI counterparts:
> 
> BridgeDestroy
> BridgeKick
> BridgeTechnologyList
> BridgeTechnologySuspend
> BridgeTechnologyUnsuspend
> 
> In the case of BridgeKick, to simplify the coding both a BridgeUniqueid and a Channel name must be provided to kick a channel.
> 
> The Technology calls are implemented in bridge.c due to the technology link list being located there.
> 
> Also in bridge.c, a new function ast_bridge_find_by_id() is added to expose the bridges ao2 container in a convenient fashion.
> 
> Tests for these AMI commands are here: https://reviewboard.asterisk.org/r/2975/
>  
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/manager_bridges.c 402126 
>   /branches/12/main/bridge.c 402126 
>   /branches/12/include/asterisk/bridge.h 402126 
> 
> Diff: https://reviewboard.asterisk.org/r/2973/diff/
> 
> 
> Testing
> -------
> 
> All tests written passed to insure basic functionality.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131030/b0333e2c/attachment.html>