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

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

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


Patch Set 8:

(8 comments)

https://gerrit.asterisk.org/#/c/1631/8/CHANGES
File CHANGES:

Line 21:    to an existing bridge containing the named bridgechannel prefix.
...named channel prefix...


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

Line 45: 			Join a bridge that contains the specified bridged channel.
...specified channel.


Line 81: 	bridge_channel = ast_channel_get_bridge_channel(c_ref);
       : 
       : 	if (!bridge_channel || !bridge_channel->bridge) {
Use ast_channel_get_bridge() to get the bridge the channel is currently in.

Please be aware that c_ref needs to be locked before calling and you also need to do an ao2_ref(bridge, -1) when done.


Line 84: 		ast_log(LOG_WARNING, "Channel %s is not in a bridge\n", data);
Use ast_channel_name(c_ref) instead of data since you have a specific channel which might not be the expected channel.


Line 89: 	if (ast_bridge_features_init(&chan_features)) {
       : 		ast_bridge_features_cleanup(&chan_features);
       : 		ast_channel_unref(c_ref);
       : 		return -1;
       :         }
This leaks bridge_channel ref.


Line 95: 	ast_verb(3, "%s is joining bridgechannel %s:%s\n", ast_channel_name(chan), data, bridge_channel->bridge->uniqueid);
How about:
ast_verb(3, "%s is joining %s in bridge %s.\n", ast_channel_name(chan), ast_channel_name(c_ref), bridge->uniqueid);

Guidelines: This line needs to be wrapped at 90.


Line 97: 	if ( ast_bridge_join( bridge_channel->bridge, chan, NULL, &chan_features, NULL, 0 ) ) {
Guidelines: Remove the extra spaces around the parentheses.

It is not a good idea to keep the c_ref reference when calling ast_bridge_join().  The c_ref reference can keep the channel around long after it has hung up.  Since the only thing needed for the c_ref channel after getting the bridge is the channel name, you should save the name off into a local variable for reference in verbose and log messages.

c_name = ast_strdupa(ast_channel_name(c_ref));


Line 98: 		ast_log(LOG_WARNING, "Failed to join '%s' for '%s'.\n", data, ast_channel_name(chan));
ast_log(LOG_WARNING, "%s failed to join %s in bridge %d.\n", ast_channel_name(chan), ast_channel_name(c_ref), bridge->uniqueid);


-- 
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: 8
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alec Davis <sivad.a at paradise.net.nz>
Gerrit-Reviewer: Alec Davis <sivad.a at paradise.net.nz>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes