Summary:	ASTERISK-02673: [patch] Stack applications
Reporter:	Tilghman Lesher (tilghman)	Labels:
Date Opened:	2004-10-25 13:26:32	Date Closed:	2008-01-15 15:55:02.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20051108__pushvar.diff.txt ( 1) app_stack.c
Description:	These are the Stack applications announced at Phreaknic 8, and released today.  Included are Gosub, Return, StackPop, and GosubIf. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.
Comments:	By: Olle Johansson (oej) 2004-11-04 14:11:56.000-0600 Corydon, this looks really nice. Wonderful addition. Do we have to modify app_dial and other apps/channels to read these local variables as well as the standard channel variables? /O By: Tilghman Lesher (tilghman) 2004-11-04 14:49:00.000-0600 oej:  Not as such, no.  The local variables are accessed via the ${LOCAL(varname)} construct, which you may notice is similar to the ENV and LEN constructs.  This is related mostly to the required 2278 patch, not this one. Since variables are dereferenced in the dialplan, not in individual applications, nothing additional is necessary.  However, applications would need to modified if you intended them to _set_ local variables, not just use them. My perception is that local variables ought to be special and variables should only be considered local, if the user explicitly wants them set that way.  Otherwise, how does a user set a variable that she wants to remain beyond the exit of the current stack entry, but does not want to become a global variable? By: twisted (twisted) 2004-11-17 23:01:15.000-0600 Corydon, Could possibly provide some usage examples so that people can understand exactly *WHAT* is going on here?  This may be the reason this one fell to the bottom of the bowl By: Tilghman Lesher (tilghman) 2004-11-18 09:52:56.000-0600 As these changes are dependent upon ASTERISK-2248, the consideration of this bug is not important until 2278 gets committed. By: Tilghman Lesher (tilghman) 2004-11-18 16:10:02.000-0600 Here's an example of its use (live sample, used right now): exten => maincid,1,SetCIDNum(6153824758) exten => maincid,2,SetCIDName(VCCH\, Inc.) exten => maincid,3,Return exten => faxcid,1,SetCIDNum(6153847734) exten => faxcid,2,SetCIDName(VCCH\, Inc. Fax) exten => faxcid,3,Return ; External local exten => _9NXXXXXX,1,GosubIf($[${CHANNEL:4:2} = 43]?faxcid,1:maincid,1) exten => _9NXXXXXX,n,Dial(${TRUNK}/${EXTEN:1},,T) ; External long distance exten => _91NXXXXXXXXX,1,GosubIf($[${CHANNEL:4:2} = 43]?faxcid,1:maincid,1) exten => _91NXXXXXXXXX,n,Dial(${TRUNK}/${EXTEN:1},,T) ; External international exten => _9011.,1,GosubIf($[${CHANNEL:4:2} = 43]?faxcid,1:maincid,1) exten => _9011.,n,Dial(${TRUNK}/${EXTEN:1},,T) edited on: 11-18-04 17:24 By: Mark Spencer (markster) 2004-11-19 00:56:43.000-0600 I still think this is still very similar to macros and i don't see the need for having two constructs, e.g. MacroIf would seem to make more sense. By: petersv (petersv) 2004-11-19 01:31:49.000-0600 The local variables are a welcome extension. I don't think they are possible with macros as subroutines. Especially the parameters are clobbered by nested macros. Or did I miss something? By: Tilghman Lesher (tilghman) 2004-11-19 08:44:51.000-0600 Okay, let's kill off Macro then. By: jjhuff (jjhuff) 2004-11-19 18:01:28.000-0600 Ok, a few comments: 1. I like the arg passing of Macro 2. I don't know how Macro is implemented, but I've heard that there is some code duplication. Call stacks are pretty clean. 3. I dislike StackPop and I'm not sure if it's all that useful.   4. Return value support would be handy, but it's really just a bit of syntatic sugar. 5. I never liked how macros were basically special contexts.  I'm not sure I like them being 'special' extensions either.  However, it is handy to have them be specific to a context 'scope'. 6. All of these *If annoy me to some extent.  Maybe it's time for If()? 7. I think LOCAL(var) should only look in the current stack frame.  It confuses lexical scope and stackframes.  Argument support would remove any need to access a parent stackframe. Gosub/return *feels* cleaner to me from a language and usability point of view.  It also seems like (with some work) macro cound be implemented interms of Gosub/Return. By: Olle Johansson (oej) 2004-12-12 15:54:53.000-0600 We need to come to a decision here, not just let this code hang. What is the status? Does this apply to CVS, is Corydon76 still promoting this solution, what does Mark think about it? /housekeeping By: Tilghman Lesher (tilghman) 2004-12-12 18:26:34.000-0600 The status is that we're waiting for the prerequisite 2278.  I actually don't think we need to kill off Macro, but I do think that giving people more options for doing subroutines is a good thing. By: Olle Johansson (oej) 2004-12-27 15:56:59.000-0600 Moving this to experimental features for now. By: twisted (twisted) 2005-01-09 22:30:07.000-0600 it doesn't seem as if 2278 is having a good time.  If 2278 doesn't make it, what is to come here? By: Tilghman Lesher (tilghman) 2005-01-09 22:34:57.000-0600 If I concede on 2278, which I haven't yet, then local variables would get stripped from this patch. By: Clod Patry (junky) 2005-03-03 20:28:36.000-0600 Anything has moved here ? like 2 months of inactivity now. :( By: Matt O'Gorman (mogorman) 2005-03-17 07:34:41.000-0600 Guys if this is going to be rejected from the base lets close it.   it has no need to simply rot on the tracker. By: Kevin P. Fleming (kpfleming) 2005-03-17 09:39:22.000-0600 I still like this work quite a bit, including the support for local variables. I'd like to see this patch redone to also include a reimplementation of Macro() using this technique, since it's much lighter weight and less code duplication than what we have now. That resolves the "this is the same as macros" issue, since we keep Macro() around for backwards compatibility, but start using Gosub/Return in new deployments. By: Tilghman Lesher (tilghman) 2005-03-29 11:41:36.000-0600 kpfleming:  The stack apps can do everything Macro can do, but the exiting is a little tricky.  With Macro, when there are no more steps, the application exits.  With Stack, we only return with an explicit Return.  One way I could emulate Macro is to insert an explicit MacroExit into the dialplan when Macro is called at the end of the dialplan steps.  However, that's tricky, since there are multiple possible points of exit in a Macro. Do you see any better way of doing this? By: Kevin P. Fleming (kpfleming) 2005-03-29 15:00:12.000-0600 Ahh... MacroExit, my quick hack to save counting dialplan priorities :-) I understand your concern, I had not thought thoroughly about that problem. I don't think your suggestion would be easily implementable, because it's perfectly valid to do something like this (even though it's ugly): [macro-ugly-ugly-ugly] exten => s,1,NoOp exten => s,2,Dial(...) exten => s,3,Voicemail(...) exten => s/NXXNXXXXXX,1,Whee() exten => s/011NXXNXXXXXX,1,Whoo() In this case, both of the s/* extensions 'end' with priority 1, but execution would actually follow into s priority 2, and not exit the macro. Determining the actual exit points from a macro is definitely non-trivial. I'll have to give this some more thought to see what's possible. By: Tilghman Lesher (tilghman) 2005-03-29 15:56:58.000-0600 kpfleming:  actually, your example case would be fairly easy: First, we loop through ast_walk_contexts() and compare ast_get_context_name() against our macro name.  Once, we find the right context, then we loop through ast_walk_context_extensions(), then walk through ast_walk_extension_priorities(), finding each gap between priorities, and calling ast_add_extension to add an explicit MacroExit at each gap (assuming there isn't one already, which we'd have to check to prevent us adding a hundred priorities everytime we enter the Macro). If that's all it was, it'd be a bit complex, but doable.  Unfortunately, there's also the possibility of a Goto (or GotoIf) which jumps outside of the macro context as well as includes.  We also have possible extension patterns that we'd need to match against.  This is what would make it a bit tricky. By: Kevin P. Fleming (kpfleming) 2005-03-29 16:04:50.000-0600 I think maybe you missed the point of my contrived example... here's another one: [macro-ugly-stepchild] exten => s,1,NoOp exten => s,2,Dial(...) exten => s/_NXX,3,Foo(1) exten => s/_011NXX,3,Foo(2) exten => s/_.,3,Foo(3) exten => s,4,Voicemail(...) (not that I'd ever use anything like this) Walking this context would show a "gap" between s-2 and s-4. It would also show "gaps" after the three s/*-3 steps. However, none of these are actually gaps, there is a complete sequence through this macro without gaps. I don't think Goto() is a problem, unless Goto() is disallowed inside a Gosub()/Return(). If so, then there's no way to implement macros as a derivation of Gosub()/Return() at all, I'd think. By: Tilghman Lesher (tilghman) 2005-03-29 16:10:07.000-0600 No, I haven't missed your point, because internally, priorities are all stored ordinally, per extension.  (Doesn't matter if that's the way they were in extensions.conf or not.)  We'd just need to iterate over the list of (ordered) priorities to find gaps. By: Tilghman Lesher (tilghman) 2005-03-29 16:21:23.000-0600 And Goto() isn't a problem IFF the Goto stays inside the Macro context.  If it jumps out, then we need to figure out where it goes (what context/extension/priority) so we can trace that and insert the necessary MacroExit in that logic, as well. By: Kevin P. Fleming (kpfleming) 2005-03-29 16:23:45.000-0600 Oh, right, I see your point now. The CID matching I'm throwing in there doesn't matter, all the s-3 steps would be listed one after another in the context internally, regardless of the CID matches. Then I agree, it would be possible to walk through the context and look for gaps, adding MacroExit() (i.e. Return()) at the proper places. This could be done at load time, so it's only done once, and there's no risk of adding them multiple times. The only downside to this would be if someone is using a context named macro-foo that is _not_ used as a target for Macro(). Then again, adding MacroExit()/Return() to that context would not cause any harm either. By: Kevin P. Fleming (kpfleming) 2005-03-29 16:25:48.000-0600 Hm... On the Goto() issue, that seems very difficult to handle. That Goto() target could be an area of the dialplan that is not always used that way, so adding stuff to it would be less than optimal. Would it just be easier to have Goto() that goes outside the macro/subroutine do an implicit MacroExit()/Return(), then continue at the Goto() target location? By: Tilghman Lesher (tilghman) 2005-03-29 17:58:54.000-0600 Adding a Return to a context that doesn't currently have it actually WOULD cause harm.  Consider:  if the programmer intends the end of logic to be implicitly waiting for an extension and you change that logic to not wait for an extension but instead to Return, that's unexpected behavior.  This ignores the current behavior that if you encounter a Return without having a frame on the stack, that's considered a fatal condition (ends the call).  So the change would have to occur only to contexts which are invoked with Macro. And handling Goto's as an implicit Return is also bad.  That would mean that we cannot have any branching logic within a subroutine, which is something I, indeed, already have. The more I think about this, the more I think we should simply have people define explicitly where they want their Macros to return with a MacroExit, and skip the whole can of worms with trying to do this. By: Kevin P. Fleming (kpfleming) 2005-03-29 21:01:00.000-0600 Fully agreed. At best, if Gosub/Return go into CVS, then we can mark app_macro 'deprecated' and try to convince people to use the more efficient and easier-to-program Gosub/Return method. By: Michael Jerris (mikej) 2005-04-17 22:45:15 Is this good to go in, with a warning that macro is depretiated? By: Olle Johansson (oej) 2005-05-02 11:27:26 Declaring app_macro deprecated is a big thing. It's the only thing we have in 1.0 and there's a lot of dialplans out there that use it... I belive it has to be in there for a few versions until it's actually disabled. By: Tilghman Lesher (tilghman) 2005-05-02 11:52:44 I may need to address jjhuff's concerns and add argument passing to the Gosub applications.  That's probably the only issue left before we can commit this. By: Michael Jerris (mikej) 2005-05-06 11:34:00 corydon from IRC: [12:27] <Corydon-w> MikeJ[Laptop]: It'll run as-is, but I think I want to add arguments and return values [12:28] <Corydon-w> Or not... I dunno yet [12:28] <Corydon-w> I think we should probably go with the current implementation, rather than trying to rush in arguments and return values and get a broken implementation [12:29] <Corydon-w> i.e. we'll add those things later So this is ready for final review.  Let's get it in there as is and perhapse hold off on the depretiation of macro until we get the additional features as well.  But it is useful as is. Oh, and Corydon will probably post when he has the patches updated :) By: Tilghman Lesher (tilghman) 2005-05-06 11:53:46 I was waiting to post that note until I got the patches updated to current CVS.  ;-)  Now they are. By: Kevin P. Fleming (kpfleming) 2005-05-15 02:03:17 Notes (some of them are nitpicky, granted): - There are strncpy calls that don't subtract 1 from the buffer size, leading to potentially unterminated strings. ast_copy_string() can be used instead. - There are three instances of the code below, can it be put into a single function? headp = node->vars; while (!AST_LIST_EMPTY(headp)) { vardata = AST_LIST_FIRST(headp); AST_LIST_REMOVE_HEAD(headp, entries); ast_var_delete(vardata); } free(headp); - Please don't use %i with sscanf unless you explicitly need to support non-base-10 input. - There is a memory leak here (node is not freed): if ((priority = ast_findlabel_extension( chan, gcontext ? gcontext : chan->context, (gexten && strcasecmp(gexten, "BYEXTENSION")) ? gexten : chan->exten, gpriority, chan->cid.cid_num)) < 1) { ast_log(LOG_WARNING, "Priority '%s' must be a number > 0, or valid label\n", gpriority); return -1; } - Be careful with this "priority - 1' here... we recently found that this was a problem when Goto() was used in a macro executed from the Dial() application, and the same problem could happen here if Gosub() is called that way. if (gcontext) { strncpy(chan->context, gcontext, sizeof(chan->context) - 1); } if (gexten) { strncpy(chan->exten, gexten, sizeof(chan->exten) - 1); } chan->priority = priority - 1; - You support LOCAL() setting a variable on the channel itself (when called outside of any Gosub()), but not retrieving the same. I think it should it refuse to set anything if there is not an open Gosub() stack, but that's just my opinion :-) Do we have a final opinion from Mark on whether he wants this to be merged or not? I want to get it done as soon as possible, so it can be in place for 1.2. By: Michael Jerris (mikej) 2005-05-15 07:40:43 kevin, mark was not convinced this was necessary over macro.  Can you please discuss it with him. By: Tilghman Lesher (tilghman) 2005-05-16 00:12:29 - Fixed strncpy() calls. - I found two of this instance in the app code.  I have consolidated the code to remove one of them.  I don't know if it's wise to make the free_node code a public API. - Changed the sscanf to use %d. - Memory leak fixed. - Not really a problem.  Two points: a) when the application exits, the main loop in pbx.c immediately increments the priority b) When we store the priority, we actually store priority + 1, then set priority - 1 at return time.  Technically, I could avoid this unnecessary increment/decrement, but I modelled the behavior on the idea that the stack should store the address to which a Return would go to, not the last priority before it.  It's cosmetic and nitpicky, but it shouldn't make a difference. - Good point.  Removed. By: Kevin P. Fleming (kpfleming) 2005-05-16 01:02:52 I believe after further consideration that the priority +1/-1 problem will not affect these applications at all; that problem was due entirely to Goto() (and its variants) being able to be used on channels that are not either in a PBX or a Macro, and having to compensate for that. Gosub() will never be called on a channel that is not in either a PBX or a Macro, so there should be no issue. By: Michael Jerris (mikej) 2005-07-25 22:00:45 where do we stand on this.  This either needs to get some traction or die. By: Tilghman Lesher (tilghman) 2005-07-26 18:47:58 Updated patch to current CVS.  Hopefully this can go in before 1.2. By: Tilghman Lesher (tilghman) 2005-11-03 11:41:28.000-0600 Mega-simplified version, without local variables.  Can we get this one in before 1.2? By: Tilghman Lesher (tilghman) 2005-11-08 17:05:25.000-0600 Updated to use a different method of storing stack frames, per Mark and Kevin. By: Kevin P. Fleming (kpfleming) 2005-11-08 18:47:04.000-0600 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:55:02.000-0600 Repository: asterisk Revision: 7034 U   trunk/ChangeLog U   trunk/apps/Makefile A   trunk/apps/app_stack.c U   trunk/include/asterisk/pbx.h U   trunk/pbx.c ------------------------------------------------------------------------ r7034 | kpfleming | 2008-01-15 15:55:01 -0600 (Tue, 15 Jan 2008) | 2 lines issue ASTERISK-2673 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=7034