[Asterisk-code-review] ARI: Allow for early bridging on dialed ARI channels. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ARI: Allow for early bridging on dialed ARI channels.
......................................................................


Patch Set 1: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/#/c/2620/1/include/asterisk/bridge_channel.h
File include/asterisk/bridge_channel.h:

Line 407: int ast_bridge_channel_write_frame(struct ast_bridge_channel *bridge_channel, struct ast_frame *fr);
No doxygen?


https://gerrit.asterisk.org/#/c/2620/1/main/bridge_channel.c
File main/bridge_channel.c:

Line 1043: 	return bridge_channel_write_frame(bridge_channel, fr);
Why not just rename this function to ast_bridge_channel_write_frame() and make it non-static?


https://gerrit.asterisk.org/#/c/2620/1/main/dial.c
File main/dial.c:

Line 517: 		ast_bridge_unsuspend(bridge, chan);
See ASTERISK-21271 about concerns for _external_ threads suspending/unsuspending channels in bridges.  ConfBridge gets away with this because the thread suspending the channel is the bridge channel's thread.

If a bridge channel is in the process of leaving the bridging system then the call to ast_bridge_suspend() would not work and there isn't a way for the caller to know he didn't get control.

This is another case for the need to eliminate stasis's misuse of departable channels.  All stasis channels would then enter and stay in the bridging system.  If someone forces a channel to leave stasis, any channel currently being dialed would need to die anyway.

You could gain control of the channel in the bridge by using ast_bridge_channel_queue_callback() to handle the dial functionality.

We probably need to discuss ways and means to get this functionality accomplished.


Line 535: 			unsuspend_bridge(channel, original);
You probably want to set soft-hanup before unsuspending the channel.


Line 636: 				unsuspend_bridge(channel, channel->owner);
You probably want to set soft-hanup before unsuspending the channel.


Line 648: 				unsuspend_bridge(channel, channel->owner);
You probably want to set soft-hanup before unsuspending the channel.


Line 812: 				unsuspend_bridge(channel, channel->owner);
You probably want to set soft-hanup before unsuspending the channel.


Line 955: 				unsuspend_bridge(channel, who);
You probably want to set soft-hanup before unsuspending the channel.


Line 997: 				unsuspend_bridge(channel, channel->owner ?: who);
You probably want to set soft-hanup before unsuspending the channel.


PS1, Line 994: 			int hangup_channel;
             : 
             : 			if (channel->bridge_suspended) {
             : 				unsuspend_bridge(channel, channel->owner ?: who);
             : 				ast_channel_cleanup(channel->owner);
             : 				hangup_channel = 0;
             : 			} else{
             : 				hangup_channel = 1;
             : 			}
This is too early.  After unsuspending a channel you have no control over the channel anymore.


Line 1036: 				unsuspend_bridge(channel, channel->owner);
You probably want to set soft-hanup before unsuspending the channel.


PS1, Line 1035: 			if (channel->bridge_suspended) {
              : 				unsuspend_bridge(channel, channel->owner);
              : 				ast_channel_unref(channel->owner);
              : 				hangup_channel = 0;
              : 			} else {
              : 				hangup_channel = 1;
              : 			}
This is too early.  After unsuspending a channel you have no control over the channel anymore.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3b05862644ca5080c7743a1b9b22465486e56ba
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes