Summary:	ASTERISK-02248: [patch] Allow functions to be set
Reporter:	Tilghman Lesher (tilghman)	Labels:
Date Opened:	2004-08-20 15:27:08	Date Closed:	2011-06-07 14:04:44
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20050323__pbx_setfunc.diff.txt
Description:	By allowing functions to be set as well as retrieved, we permit the possibility of having triggers to logic that would not otherwise be accessible from the dialplan. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.
Comments:	By: twisted (twisted) 2004-08-20 16:05:50 I like the idea here, this would also allow for custom virtual variables to be established on an as-needed basis.   :) By: Tilghman Lesher (tilghman) 2004-08-24 00:41:05 Existing virtual variables have been moved into their own module in the latest patchset (both files are required, as one is a patch; the other is additional files to be created). By: Tilghman Lesher (tilghman) 2004-09-23 23:53:40 Updating patch to include recently added CALLINGPRES virtual variable. By: Tilghman Lesher (tilghman) 2004-10-03 10:04:27 Updating patch, to keep it current. By: Tilghman Lesher (tilghman) 2004-10-25 13:22:51 Updating patch, to cleanly apply against CVS.  Also including an enhancement to app_cut.c that will allow you to do a Cut inline in the dialplan, instead of as a separate step (another example of why this patch should be applied). By: Tilghman Lesher (tilghman) 2004-10-27 20:04:44 Ditto for DBGet, a way to do a DBGet within a dialplan line, without making it a separate step.  This is more a proof-of-concept example, since I believe you still need the error-checking provision of the DBGet app, but it's interesting nonetheless (and demonstrates the true power of the main patch). By: Tilghman Lesher (tilghman) 2004-11-04 11:44:15.000-0600 Ooops, fixing a typo in the unregister of variables... By: Brian West (bkw918) 2004-11-30 00:12:33.000-0600 Please hound kram to either get this in or let him tell you what is wrong with it to get it in ;) By: twisted (twisted) 2004-12-15 20:36:21.000-0600 This needs to get moving again, otherwise it will be closed for lack of interest. --Housekeeping By: Tilghman Lesher (tilghman) 2004-12-16 00:17:03.000-0600 Reminder sent to markster We really need to know what is keeping this patch out of CVS. By: Tilghman Lesher (tilghman) 2004-12-27 17:51:42.000-0600 Patches reuploaded.  The only two existing variables I have left separate are LEN and ENV (based partially upon a conversation with Mark). I have some additional functionality to implement in pbx.c, but the current changes are tested and working. By: khb (khb) 2004-12-28 16:04:11.000-0600 Some documentation on how this is used would be nice. Otherwise it will remain as obscure as most of those variables to most users. What are 'virtual' variables anyways? Variables that a readonly and set by the PBX ? By: Tilghman Lesher (tilghman) 2004-12-28 23:49:29.000-0600 I'll consider writing some documentation, but this isn't really something users need to be concerned with -- it's really a development feature, intended to allow other developers to hook their own routines into the core variable code. By: khb (khb) 2004-12-29 01:49:43.000-0600 Yes, and all the more reason to document it. Developers need documentation also, even more than users, otherwise they won't use such features and continue to cook their own methods. This seems like something generally useful, that I can see using potentially, but not if it's another obscure features whose intent is soon forgotten, in which case it's just as easy to add variables the way it's done now, elegent or not. By: Mark Spencer (markster) 2005-01-06 22:20:38.000-0600 I don't think this is going to make it into CVS for reasons I've discussed with corydon (mainly that I don't see it having a sufficient benefit to merit its complication).  However corydon can still contact me if he wants to try to reopen this for any reason. By: Tilghman Lesher (tilghman) 2005-01-06 22:27:49.000-0600 Did you look at the latest revision?  I've made most of the concessions to the patches that you requested. By: twisted (twisted) 2005-01-28 19:49:06.000-0600 Is this going anywhere?  I see that markster made the comment that you need to contact him regarding this if you wanted to reopen; How did this go?  It's been sitting around for quite some time now.. By: Tilghman Lesher (tilghman) 2005-01-28 23:25:48.000-0600 Mark made some very pointed criticisms to this patchset, and I've rolled back most of the radical changes that he objected to.  At this time, we're waiting to hear if we've gone far enough or if I still need to rollback some of the changes. I think the idea is sound overall and would be very beneficial to be included in CVS. By: Kevin P. Fleming (kpfleming) 2005-03-03 15:48:00.000-0600 Can this be updated to match current CVS so we can get it in? I'd like to see this get into CVS soon, as it is very useful and provides modularity. By: Mark Spencer (markster) 2005-03-04 00:51:28.000-0600 Does the merger of the functional variables $(foo) supercede this? By: Kevin P. Fleming (kpfleming) 2005-03-04 08:36:32.000-0600 I don't think so, since the syntax is different. I believe the whole idea of this patch was to allow easier implementation of standard variables with normal syntax (and also allow modular loading of the existing ones when useful). Granted, the implementation is very similar, but the overhead for these is a bit lower because there's no parsing for arguments and such to be passed to them, since they are not functions. By: Mark Spencer (markster) 2005-03-11 00:45:17.000-0600 corydon76: does tonys' $(foo) function handling fit your needs? By: Tilghman Lesher (tilghman) 2005-03-11 16:38:08.000-0600 It almost does.  However, it's missing a key feature that I didn't plan on originally, but which my current implementation has.  That is, virtual variables with this patch allow a write to be trapped.  It acts like a trigger, which allows the system to react immediately to a change of a variable, rather than continually polling the value of a variable to see if it has changed. I think that ability to trap a write to a variable means that this patch is either still valuable or else functions could be altered to allow writes to be trapped with that implementation.  Either way, I'd be happy. By: Brian West (bkw918) 2005-03-16 17:40:35.000-0600 I think the consensus is this isn't going in.... /b By: Mark Spencer (markster) 2005-03-16 18:24:12.000-0600 Perhaps, but the ability to capture writes to $(foo) would also potentially be useful, allowing the replacement of the SetFoo and GetFoo applications.  How about we allow this to remain open one more week pending a patch from corydon76 to add write capability to $(foo) if registered with both a "read" and "write" function? By: Tilghman Lesher (tilghman) 2005-03-17 13:12:25.000-0600 markster:  patch as requested. By: Kevin P. Fleming (kpfleming) 2005-03-17 13:55:33.000-0600 This patch looks fine to me; keep in mind that the reason it is so large is that it includes a lot of whitespace/formatting fixes (all good changes). The actual SetFunction() code is pretty small. By: Tilghman Lesher (tilghman) 2005-03-17 16:07:10.000-0600 Fixed conflicts with $(if) patch By: Mark Spencer (markster) 2005-03-20 02:07:58.000-0600 I'd like to get anthm's input here although it looks pretty good to me at least at a quick read through. By: Mark Spencer (markster) 2005-03-20 02:08:26.000-0600 Note: applying the patch and then performing cvs diff -u -w allows you to see all non-whitespace changes. By: Mark Spencer (markster) 2005-03-20 11:45:01.000-0600 What if setvar detects the variable name begins with "(" and then interprets it as a function so we don't have to have setvar/setfunction as separate apps?  e.g.: SetVar((sip username)=asdf) By: Anthony Minessale (anthm) 2005-03-20 11:48:55.000-0600 sounds like something we discussed on thursday seems ok to me. By: Kevin P. Fleming (kpfleming) 2005-03-20 11:51:14.000-0600 Corydon, anthm and I discussed this on IRC a few days ago, and we tried to come up with a syntax that made sense and would be easy for people to understand. I would be concerned about using SetVar() for this, because extracting the value back out does not use variable syntax, but function syntax. This could easily lead to confusion. Personally, I would much prefer a solution like the one that Corydon originally proposed, where you could do this: ${VAR} - gets value of variable ${FUNC(arg1 arg2 ...)} - runs function with args and replaces result into string SetVar(VAR=value) - sets variable value SetVar(FUNC(arg1 arg2 ...)=value) - runs function in 'write' mode and supplies value This seems much more consistent and more in line with traditional languages. By: Anthony Minessale (anthm) 2005-03-20 12:18:27.000-0600 Doesnt seem unreasonable but it would cause more work because every existing dereference to a variable would have to check for a '(' too to make sure it's not a function which is why I picked a unique $( Sometimes these syntax things are harder than the code =D Here are some more: * This one the check is easier cos you gotta find '}' anyway so you just see if the next char is another '{' or not. ${FUNC}{args} SetVar(FUNC{args})=foo) * This one make it look a little perlish but might be harder to parse. &func(args) SetVar(&func(args)=foo) By: Kevin P. Fleming (kpfleming) 2005-03-20 12:24:34.000-0600 Yeah, the first alternative (a second group of elements inside {}) looks good to me, it's fast to parse and easy to understand. By: Tilghman Lesher (tilghman) 2005-03-20 19:25:41.000-0600 anthm:  I really don't think doing ${FUNC(args)} is really any more work, in that it doesn't significantly complicate the search.  It's a linear search now, and it would still be a linear search. We already have two builtin "variables" that are really, at their soul, functions:  ENV() and LEN().  Adding more functions doesn't really complicate things.  I would make the comment, however, that we should probably move ENV and LEN over to being core functions, not core variables, for consistency (also, it means that we can set environmental variables with SetVar, which we cannot do now). Consistent syntax (between variables and functions) means users don't have to learn a new syntax and don't have to keep their minds straight about which is which.  "LEN does a calculation, so therefore we use function syntax... no wait, that's one of the exceptions, right?  so we use variable syntax.  Or wait...  Darn, I guess I'm going to have to go look it up again." So that lets us do the following syntaxes (my original proposal): ${foo} SetVar(foo=blah) ${fu(bar)} SetVar(fu(bar)=blah) It doesn't complicate syntax, is easier to remember, and doesn't alter the underlying code complexity (either for better or for worse). By: Kevin P. Fleming (kpfleming) 2005-03-20 19:37:49.000-0600 anthm's point was that telling the difference between a variable name and a function name requires searching for a '(' within the string with that syntax (although since I also proposed it, obviously I think it's the right way to go). Putting the arguments _after_ the '}' is essentially zero-cost, since we already have to search for that anyway. Once it has been determined whether the string is a variable name or a function name, your point is valid, it doesn't take any more time to figure out what to do with it. For now, we'd leave the function invocation stuff the way it is (a separate list from variables), but just use this new syntax to trigger it instead. If at some point down the road we can convince Mark that moving the built-in variables into this list (and making ENV and LEN functions) is the right thing to do, then at that point we'd gain the benefits of your original patch (easier addition of new variables by modules, wholesale replacement of existing variables, etc.) By: Kevin P. Fleming (kpfleming) 2005-03-20 19:39:12.000-0600 I've been waiting for this to shake out because I want to finish up ASTERISK-3313346, but I hate having to add an application for SetPreferredCodec when using SetVar(PREFERRED_CODEC=value) would be much better, if there was a way to "grab" it and store the value in a dedicated variable, instead of the linked-list. By: Mark Spencer (markster) 2005-03-20 23:28:58.000-0600 I'm happy with the ${FUNC(args)} and don't beleive it's a performance hit because when we find the '}' we can likewise determine it is a function by the presense of teh ')' just before it, without performing a linear search from the start for '(' until after we already know it's a function.  I'd be happy to see ${ENV(foo)} et al be moved to this sort of function, since thats more appropriate to what they are anyway.  This also could replace a number of specialty applications as well. By: Kevin P. Fleming (kpfleming) 2005-03-20 23:35:53.000-0600 Yay! Thanks for the hint about looking backwards for the ')' character... of course that will work just fine. When the new patch comes, should it continue to support the short-lived $(...) syntax (marking it deprecated), or just remove it since it was in for only a couple of weeks? By: Anthony Minessale (anthm) 2005-03-21 08:38:25.000-0600 I concur, eagerly awaiting the patch so we can have closure. =D Nobody uses the funcs yet so I say you can lose the $(foo) By: Mark Spencer (markster) 2005-03-21 20:13:49.000-0600 I agree that there is no reason to keep the $(foo) syntax. By: Tilghman Lesher (tilghman) 2005-03-23 14:15:06.000-0600 New patch, disposing of $(FOO args) in favor of ${FOO(args)}. Mark: a clarification.  In terms of replacing some of the custom apps, are you suggesting replacing apps like SetLanguage with SetVar(LANGUAGE()=en) or even SetVar(CHANNEL(LANGUAGE)=en) or was that suggestion for something else? By: Kevin P. Fleming (kpfleming) 2005-03-23 23:55:14.000-0600 In my mind it would make sense for SetVar(LANGUAGE()=fr) and SetVar(LANGUAGE=fr) to do the same thing (invoke the function in write mode with NULL for arguments), if LANGUAGE is defined as a function name. Otherwise, I'd like to see the 'virtual variables' functionality that you originally had brought back, so that I can add a handler for PREFERRED_CODEC to intercept it and store the data in a different place (not in the channel variables linked list) without having to modify pbx_builtin_setvar_helper. By: Anthony Minessale (anthm) 2005-03-24 09:15:16.000-0600 Seems reasonable but then how do we tell it's a magic var without strcmping against all the funcs everytime we set a var since the "()" offers a hint. I'll try again to plug hashes, if we add sqlite to asterisk (it's better than gpl it says the code is entirely free) we can use all thier data storage objects especially hashes besides the other benifits that are abounding. By: Tilghman Lesher (tilghman) 2005-03-24 09:21:32.000-0600 kpfleming: well, virtual variables were just another implementation of functions.  The compromise that we'd reached earlier of having the last character of the variable name be ')' in order to trigger functions would have to be scrapped, if we were to allow variables to be triggers, instead of only allowing functions to be triggers.  I really don't see any reason we should duplicate the functionality of functions with another implementation of virtual variables (which are really just functions). In terms of syntax, perhaps we should keep the builtin variables where they are now, but deprecate that syntax, asking people to start using builtin functions, instead, i.e. ${LANGUAGE()}.  Note that ${LANGUAGE} and ${LANGUAGE()} would do the same thing, here, but we'd have the additional functionality of SetVar(LANGUAGE()=en).  There could even be a transition period of the next _major_ STABLE release and we remove the builtin variables in pbx.c for the _following_ major STABLE release.  Or we keep the builtin variables, and you can access the builtins either as variables or as functions, but they can only be _set_ as functions. For PREFERRED_CODEC, since it's new, it's not a burden to tell people that they _must_ use the function syntax, i.e. ${PREFERRED_CODEC()} and SetVar(PREFERRED_CODEC()=ulaw). By: Kevin P. Fleming (kpfleming) 2005-03-24 09:51:28.000-0600 Mark: if I can get you a complete, working, hashtable-based ASTOBJ container implementation early next week, are you willing to let us use it to move _all_ virtual variables into a hashtable (thus they would all be registered and quickly findable)? This would make _all_ virtual variables into functions, effectively, and would easily allow for use with or without (). You search the hashtable first, and if you don't find it and there were no () included, then you add the variable to the linked list of channel variables. Same process in reverse for ${VAR} usage. By: Mark Spencer (markster) 2005-03-24 14:42:45.000-0600 I agree that hashing variables (the section before the opening parethesis) is the right way to go for performance and extensibility.  However, the ASTOBJ work has stretched on since long before VON and there are multiple patches that are in some level of holdup based upon the ASTOBJ stuff.  Lets see the ASTOBJ implementation AND associated programmer documentation soon so that people can more easily start to use it.  If we're not going to have ASTOBJ finalized soon, then we need to start letting these other patches find their way in and address ASTOBJ at some later time. By: Mark Spencer (markster) 2005-03-29 00:23:18.000-0600 We'll review the hashing stuff later.  I went ahead and fixed this and merged it in. By: Leif Madsen (lmadsen) 2005-03-30 10:10:40.000-0600 Is there any possibility of getting some docs for this? If someone can even just explain how it works briefly, I'll write it. By: Digium Subversion (svnbot) 2008-01-15 15:29:12.000-0600 Repository: asterisk Revision: 5296 U   trunk/cdr.c U   trunk/channels/chan_alsa.c U   trunk/channels/chan_mgcp.c U   trunk/include/asterisk/cdr.h U   trunk/include/asterisk/pbx.h U   trunk/pbx.c ------------------------------------------------------------------------ r5296 | markster | 2008-01-15 15:29:12 -0600 (Tue, 15 Jan 2008) | 2 lines Allow functions to be written to (bug ASTERISK-2248, with mods) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5296