[Asterisk-code-review] app bridgeaddchan: ability to barge into existing call (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: app_bridgeaddchan: ability to barge into existing call
......................................................................


Patch Set 3: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/1631/3/apps/app_bridgeaddchan.c
File apps/app_bridgeaddchan.c:

Line 4:  * Copyright (C) 1999 - 2005, Digium, Inc.
Just 2015 for the Copyright date.


Line 6:  * Mark Spencer <markster at digium.com>
You should put your name here Alec.


Line 54: 			<para>This application places the incoming channel into 
Red blob on this line.


Line 78: 		ast_log(LOG_NOTICE, "no bridge\n");
This should probably be a warning, and it should be expanded. Something like "Channel <blah> is not in a bridge" would be good enough.


Line 83: 	if ( ast_bridge_add_channel( bridge_channel->bridge, chan, NULL, 0, NULL ) ) {
Using ast_bridge_add_channel() is going to cause big problems unless BridgeAdd is the very last operation in the dialplan. The problem is that you're going to end up with two threads that think they are the owners of the channel. Instead, you'll need to use ast_bridge_join() here so that the call will block until the channel has left the bridge.


Line 84: 		ast_log(LOG_NOTICE, "Failed to join bridge\n");
This is another notice that should be a warning instead.


Line 85: 		ao2_cleanup(bridge_channel->bridge);
I think this should just be ao2_cleanup(bridge_channel).


Line 90: 	ao2_cleanup(bridge_channel->bridge);
Same here, just ao2_cleanup(bridge_channel)


-- 
To view, visit https://gerrit.asterisk.org/1631
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8eb5096a02168dcc8d7aeea416ef36ba4ed10540
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alec Davis <sivad.a at paradise.net.nz>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes