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

[Kevin Harwell]

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:

(4 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@1416
PS6, Line 1416: 								break;
> I think these are both ok. […]
yes you are right about "msg". It actually breaks out of the for loop, and not the while loop, so is okay as it does get unref'd. And adding the unref for next_app at the end of the loop should also take care of this case.


https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1419
PS6, Line 1419: 						if (r == 0) {
> I think it needs to be there, strictly for that error scenario. […]
No clue what I was thinking. You're correct.


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.
> The main reason was to do away with having to have a variable named "app" inside of the stasis_app_e […]
Makes sense you are trying to remove the variable, but I lean toward keeping it for the sake of code consistency and slightly less obfuscation. But that's just me if you feel strongly then at least you've documented the behavior.


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;
> Posting what we talked about in stand-up for public visibility: stasis start requires a parameter of […]
Can you use "AST_VECTOR_STEAL_ELEMENTS"? If you're moving from one to the other the old app no longer needs the arguments. Just "steal"/dump them pass in and create add as a vector in the new app?



-- 
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: Tue, 26 Feb 2019 00:10:00 +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/f8d21d8a/attachment.html>