[Asterisk-code-review] ARI: Re-implement the ARI dial command, allowing for early b... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ARI: Re-implement the ARI dial command, allowing for early bridging.
......................................................................


Patch Set 1: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/#/c/2790/1//COMMIT_MSG
Commit Message:

PS1, Line 31: being handled by bridge channel threads.
            : 
            : Change-Id: I7750359ddf45fcd45eaec749c5b3822de4a8ddbb
Need to add
ASTERISK-25925
issue reference.


https://gerrit.asterisk.org/#/c/2790/1/res/ari/resource_channels.c
File res/ari/resource_channels.c:

Line 1598: 	if (ast_channel_state(callee) != AST_STATE_DOWN) {
Need to accept both AST_STATE_DOWN and AST_STATE_RESERVED.
ast_request() for chan_dahdi and chan_misdn creates all channels in the reserved state before ast_call().


PS1, Line 1604: 	/* XXX This is straight up copied from main/dial.c. It's probably good
              : 	 * to separate this to some common method.
              : 	 *
              : 	 * XXX Locking of the channels needs to be considered
              : 	 *
              : 	 * XXX Probably should stage a channel snapshot of the callee
You need to copy more from begin_dial_prerun() to handle your XXX items because locking is needed and the staging is needed.  Both of these concerns were handled in the code you copied from.


Line 1632: 	if (stasis_app_control_dial(control, callee, args->dialstring, args->timeout)) {
Callee does not need to be passed since it is supposed to be the channel associated with control.


https://gerrit.asterisk.org/#/c/2790/1/res/stasis/control.c
File res/stasis/control.c:

Line 340: static int app_control_continue(struct stasis_app_control *control,
This gives us the interesting possibility of continuing a channel in the dialplan that we may have dialed but has not been answered yet.

Do we care?


Line 837: static struct ast_bridge *dial_bridge;
This is leaked on shutdown.


Line 914: 	control->bridge = bridge;
Set the control->bridge before imparting it and clear it on failure.  Otherwise, you have a potential race condition of the channel leaving the dial bridge before you set the control->bridge value.


Line 1109: 	if (ast_bridge_interval_hook(bridge_channel->features, 0, ms > 0 ? ms : 1, bridge_timeout, NULL, NULL, 0)) {
break long line


PS1, Line 1354: tatic struct control_dial_args *control_dial_args_alloc(struct ast_channel *callee,
              : 		const char *dialstring, unsigned int timeout)
              : {
callee is not used


Line 1359: 	args = ast_calloc(1, sizeof(*args) + strlen(dialstring) + 1);
You could safely do an ast_malloc() here instead.


Line 1412: 	struct control_dial_args *args = data; 
red blob


PS1, Line 1434: int stasis_app_control_dial(struct stasis_app_control *control,
              : 		struct ast_channel *callee, const char *dialstring, unsigned int timeout)
callee is not needed here since it should be the channel associated with control.


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

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