Summary:	ASTERISK-21390: New applications for app_stack.c: GosubEntry and StackPopGoto
Reporter:	Corey Farrell (coreyfarrell)	Labels:
Date Opened:	2013-04-08 16:16:15	Date Closed:	2013-08-19 18:36:52
Priority:	Minor	Regression?
Status:	Closed/Complete	Components:	Applications/app_stack
Versions:	SVN	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_stack-new_apps.patch
Description:	I've created two applications for use with Gosub scripts. GosubEntry Accepts a list of variable names to be initialized as local variable with values of ${ARGn} This application is mainly to reduce the verbosity of Gosub's created by a GUI.  Instead of setting local variables for each argument separately it allows all to be set by a single priority. {code} exten => s,1,Set(LOCAL(NUMBER)=${ARG1}) same => n,Set(LOCAL(STATUS)=${ARG2}) same => n,Set(LOCAL(RESULTS)=${ARG3}) ; Can be rewritten: exten => s,1,GosubEntry(NUMBER,STATUS,RESULTS) {code} Verbose >= 6 can be used to have GosubEntry produce output for each variable set. StackPopGoto - runs StackPop followed by Goto.  The combined command is needed for leak-free Gosub's where the final Goto destination is in a local variable. I feel that the code for these are ready, the xmldocs need help.
Comments:	By: Rusty Newton (rnewton) 2013-04-10 18:57:52.198-0500 Corey, do you have access to reviewboard? If you do then you may want to go ahead and post it there assuming you've reviewed it against the coding guidelines and had someone else test it. https://wiki.asterisk.org/wiki/display/AST/Reviewboard+Usage By: Corey Farrell (coreyfarrell) 2013-04-10 19:11:32.088-0500 I don't have reviewboard access.  I spoke to Matt Jordan a couple days ago on IRC, he said he was going to look into getting me access.  He told me to always post to JIRA first.  As for someone else testing the apps, I wrote them last month for a customer's system so they are already being used by a couple thousand channels weekly.  Not sure if that counts since I maintain the system. By: Rusty Newton (rnewton) 2013-04-17 16:53:26.508-0500 Yes, sorry it's always a good idea to post to JIRA first if you haven't submitted patches in a while or are new to writing code for Asterisk. It's always good to have someone else test who can reproduce the issue, but that knowledge is still helpful. By: Matt Jordan (mjordan) 2013-07-21 21:42:22.465-0500 Sorry for the late reply on this issue Corey. I've gone ahead and set up a Review Board account for you. I've also posted the patch up for code review. By: Tilghman Lesher (tilghman) 2013-07-22 01:12:41.338-0500 Could you explain the impetus for these two new applications?  It seems like 1) artificially reducing the verbosity of applications is a bad idea, and 2) simply combining two applications into one instance might sound like a way to teach yourself the API, but it doesn't seem like a particularly good idea. There is no end to the number of applications that could be combined for particular people's dialplans.  The nice part of the current Stack applications is that what you're proposing to do can already be done; new functionality should be confined to what you cannot already do. By: Corey Farrell (coreyfarrell) 2013-07-24 17:09:37.663-0500 GosubEntry was created to make extensions.conf and verbose logs easier to read.  It is also to remove references to $\{ARG1\}, $\{ARG2\}, etc from Gosub's.  For the same reason I use priority 'n', it's better to not deal with argument indexes. StackPopGoto is not an arbitrary combination of applications.  It was created to allow use of a local variable for the Goto destination.  Gosub scripts should not Set non-local channel variables unless doing so is a goal of the script.  I understand your concern about unending list of new applications.  Maybe StackPopExec would be better?  Cover all possible uses for out of scope local variables. By: Tilghman Lesher (tilghman) 2013-07-24 18:41:30.472-0500 Okay, then how about calling them something along the lines of "ReturnTo" (or ErrorReturn) and "RenameArgs"?  Name them something that indicates what they're doing, rather than the programming logic you went through to get there. The actual intent of StackPop was something that could be used within an error routine.  That is, you Goto your error routine, and it keeps your variables intact long enough for you to handle that error, and then you either StackPop and go to a static location, or you Return to the place that called the subroutine.  From what I'm understanding, you're removing the stack frame before you enter your error routine, which means you're losing all the contextual information that you might want to log in that routine. By: Corey Farrell (coreyfarrell) 2013-07-25 15:39:34.777-0500 I like "ReturnTo" since I'm using this for routing, not error handling.  For example I have a Gosub routine that wraps ReadExten, handles retries, finally transfers to the desired context/extension.  It does not Return() for any situation, Return() would actually send the call to an unused priority. I think "RenameArgs" sounds like it would modify or remove $\{ARG1\} etc.  My app only copies ARGn variables to the local named variables.  How do you feel about "NameArgs"? By: Tilghman Lesher (tilghman) 2013-07-25 16:17:35.028-0500 It certainly could "remove" the arguments -- just set them to blank (_not_ NULL) after you set the new name. Since you're using this for routing, I'd actually suggest that your Return($\{newcontext},$\{newextension},$\{newpriority}) and then Goto($\{RETVAL}) as your next priority.  The purpose of StackPop is exception handling, not routine abuse of the stack functionality. By: Corey Farrell (coreyfarrell) 2013-07-25 19:39:42.397-0500 I don't see any benefit to simply blanking the arguments, what if they were shifted?  This would allow support for variadic routines.  ShiftArgs? As for "ReturnTo", the point of the Gosub's using it is 1) determine where to go next and 2) go there.  Copying a Goto after every call to those Gosub routines is not a workable solution for me (one server would require an identical Goto in 70 places).  I'm not a fan of code duplication - that's a big part of why I'm using Gosub routines.  I understand that I'm using the stack in a slightly different way than originally intended, but this is a new feature request.  I don't think it's abusing anything. By: Tilghman Lesher (tilghman) 2013-07-25 19:52:31.364-0500 There's no such thing as shifting the arguments.  They're simply variables.  You can emulate that, but you might as well just blank out the variables at that point. Gosub stands for "Goto subroutine".  Subroutines are meant to return back to the place from which they were invoked.  I understand that you want to do this in unstructured ways, but that's really poor programming practice.  We try to encourage good programming practices, both in our own code, as well as in the dialplan. By: Corey Farrell (coreyfarrell) 2013-08-19 05:05:52.662-0500 I understand your arguments against including this in asterisk releases.  I'm ok with canceling this feature request.  I'm not sure how to close the review board request; maybe this is because mjordan posted it?  Sorry if I'm misunderstanding but it seems like I'm the only one who wants these apps added to asterisk. By: Matt Jordan (mjordan) 2013-08-19 18:36:52.435-0500 Retracting patch :-)