[Asterisk-code-review] res stasis: Auto-create context and add ability to switch ap... (asterisk[13])

[Joshua C. Colp]

Joshua C. Colp has posted comments on this change. ( https://gerrit.asterisk.org/10882 )

Change subject: res_stasis: Auto-create context and add ability to switch applications.
......................................................................


Patch Set 4: Code-Review-1

(9 comments)

This is an initial review, still working on it.

https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c
File res/res_stasis.c:

https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1333
PS4, Line 1333: 		if (control_next_app(control)) {
Document here that control_next_app is only modified within the control thread


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1339
PS4, Line 1339: 			if (app && app_is_active(app)) {
what happens if app is not found or app is not active? what's the behavior for the channel?

app used to point to the previous app, now it does not (leaking a reference)


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1354
PS4, Line 1354: 				/* We bumped this when trying to find it, so we need to decrement */
I think this reference counting needs to be better documented/defined. You need to drop it twice because 'app' had a reference to it which you kind of inherited/lost.


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1357
PS4, Line 1357: 				cleanup();
It should be documented why you call this


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1359
PS4, Line 1359: 				ao2_cleanup(last_app);
It's guaranteed last_app will always be non-NULL so ao2_ref can be used


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1399
PS4, Line 1399: 				app_unsubscribe_bridge(control_app(control), last_bridge);
Should the few calls at the beginning of this function also move to using control_app?


https://gerrit.asterisk.org/#/c/10882/4/res/res_stasis.c@1498
PS4, Line 1498: 	ao2_cleanup(control_app(control));
              : 	app = NULL;
I don't think app is actually necessary here for reasons previously mentioned in this review.


https://gerrit.asterisk.org/#/c/10882/4/res/stasis/app.c
File res/stasis/app.c:

https://gerrit.asterisk.org/#/c/10882/4/res/stasis/app.c@1029
PS4, Line 1029: 			ast_log(LOG_WARNING, "Could not find or create context '%s'\n", context_name);
I'd extend this warning to include details about the application too


https://gerrit.asterisk.org/#/c/10882/4/res/stasis/control.c
File res/stasis/control.c:

https://gerrit.asterisk.org/#/c/10882/4/res/stasis/control.c@1401
PS4, Line 1401: 	control->app = ao2_bump(app);
What if there's an existing app here?



-- 
To view, visit https://gerrit.asterisk.org/10882
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: Ib6c569468472dbb08905b356887373c81e03015d
Gerrit-Change-Number: 10882
Gerrit-PatchSet: 4
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 14:36:24 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190121/3f915ac8/attachment.html>