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

Tue Feb 5 15:27:27 CST 2019

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/10937 )

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

Patch Set 6: Code-Review-1

(18 comments)

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@1345
PS6, Line 1345: 						break;
ref leak on next_app

https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1375
PS6, Line 1375: 					break;
next_app being ref leaked here

https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1416
PS6, Line 1416: 								break;
msg and next_app are ref leaked here

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.

https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1427
PS6, Line 1427: 		}
next_app is being ref leaked

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 don't ref steal in cases like this.

Usually a particular scope of code is responsible for its own incrementing/decrementing the ref.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@119
PS6, Line 119:  * \note This will unref the previous app by 1, and bump app by 1
I'd add the word "control's" in this statement, so it is clear it's the control's app ref being removed. As in, "...control's previous app by 1, ...."

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@123
PS6, Line 123: char *control_next_app(struct stasis_app_control *control);
Missing documentation.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@132
PS6, Line 132: 
             : char **control_argv(struct stasis_app_control *control);
             : 
             : int control_argc(struct stasis_app_control *control);
Both of these are also missing docs.

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. It might simplify some of the other code or make it easier to read.

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 the condition init, so if it fails the control unref will clean it up without having to call ao2_cleanup directly here.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@541
PS6, Line 541: 	if (!(move_data = ast_calloc(1, sizeof(*move_data)))) {
The move_data structure eventually gets free'd by the ast_free_ptr callback, but some of the objects it "owns" do not like app_name and argv. See below.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@546
PS6, Line 546: 	if (!(move_data->app_name = ast_calloc(1, size))) {
app_name is leaked. At least I did not see where it was ever free'd

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@554
PS6, Line 554: 		if (!(iter = ast_calloc(1, size))) {
It doesn't appear "iter" is free'd. You could probably just use a stack variable here.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@560
PS6, Line 560: 		if (!(move_data->argv = ast_calloc(1, sizeof(*move_data->argv)))) {
This is never free'd

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@564
PS6, Line 564: 		while ((token = strtok_r(iter, ",", &iter))) {
I'm not 100% sure, but I don't think you want strtok_r's saveptr (last parameter) to be the same as the first parameter. Although it might not be a problem here.

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@569
PS6, Line 569: 			move_data->argv[argc] = ast_calloc(1, size);
Each element is leaked as it is copied again later and never free'd

https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@1413
PS6, Line 1413: 	if (control->next_app) {
              : 		ast_free(control->next_app);
              : 	}
Can just use ao2_cleanup, or move the setting to NULL under here.

-- 
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: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 21:27:27 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190205/7ec891a8/attachment-0001.html>