[Asterisk-code-review] res stasis: Add ability to switch applications. (asterisk[13])

[Benjamin Keith Ford]

Benjamin Keith Ford has posted comments on this change. ( https://gerrit.asterisk.org/10937 )

Change subject: res_stasis: Add ability to switch applications.
......................................................................


Patch Set 6:

(5 comments)

Still working through making all the changes

https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c
File res/res_stasis.c:

https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1416
PS6, Line 1416: 								break;
> msg and next_app are ref leaked here
I think these are both ok. msg will still be unref'd whether there was an error or not, and I've added an ao2_cleanup after control_move_cleanup which should take care of next_app in the case of it not being active but having a ref.


https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1419
PS6, Line 1419: 						if (r == 0) {
> I don't think you need this check since if "r" is ever non-zero it breaks out of the loop above.
I think it needs to be there, strictly for that error scenario. If "r" is non-zero (breaking the loop), then something went wrong and the message shouldn't be sent. It will only ever be 0 here if it makes it through the entire loop successfully.


https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h
File res/stasis/control.h:

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@40
PS6, Line 40:  *
            :  * \note This function inherits app's ref rather than bumping app!
            :  * If something goes wrong, ao2_cleanup is called on the app.
> Any particular reason for this change? To me it breaks the usual expectations a bit as we typically  […]
The main reason was to do away with having to have a variable named "app" inside of the stasis_app_exec function. It was relying on using it and "app_name" to do all function calls, when it should have been using "control_xxx" functions to get the data. This is more important now since the application can change within the control structure. The reason it now inherits the ref is because when the function is called to create the control, the parameter for "app" is passed in as an "ao2_find" and we don't actually have direct access to the app without first creating the control.


https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c
File res/stasis/control.c:

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@86
PS6, Line 86: 	/*!
            : 	 * The list of arguments to pass to StasisStart when moving to another app.
            : 	 */
            : 	char **argv;
            : 	/*!
            : 	 * The number of arguments argv contains.
            : 	 */
            : 	int argc;
> I'm thinking you can just use a string vector here instead. […]
Posting what we talked about in stand-up for public visibility: stasis start requires a parameter of type "char **" for the argument list, and if using a vector, we would need to access the raw buffer, which technically "violates" the way vectors are supposed to be used.


https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@129
PS6, Line 129: 	res = ast_cond_init(&control->wait_cond, NULL);
             : 	if (res != 0) {
             : 		ast_log(LOG_ERROR, "Error initializing ast_cond_t: %s\n",
             : 			strerror(errno));
             : 		ao2_cleanup(app);
             : 		ao2_ref(control, -1);
             : 		return NULL;
             : 	}
             : 
             : 	control->app = app;
> I'm not a fan of stealing the ref here, but if you do it this way you could move this just before th […]
I can move it up, but will wait on feedback from reasoning behind the stolen ref.



-- 
To view, visit https://gerrit.asterisk.org/10937
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I43d12b10045a98a8d42541889b85695be26f288a
Gerrit-Change-Number: 10937
Gerrit-PatchSet: 6
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 25 Feb 2019 17:10:52 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190225/07ec2492/attachment.html>