Summary:	ASTERISK-03243: [patch] SetVar should not allow readonly variables to be set
Reporter:	Tilghman Lesher (tilghman)	Labels:
Date Opened:	2005-01-10 14:28:13.000-0600	Date Closed:	2008-01-15 15:24:33.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/Configuration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20050110__setvar_builtin.diff.txt
Description:	Currently, some variables are reserved and pass special values when referenced.  However, the setvar routine allows any variable to be set, even if it cannot be later referenced. We should either set the appropriate structure or warn the user why the structure should not be altered, as appropriate, when the user attempts to set reserved variables. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.
Comments:	By: Olle Johansson (oej) 2005-01-10 14:48:53.000-0600 Good catch. What about variables like that applications set? Should we warn that these can be overwritten? By: Mark Spencer (markster) 2005-01-10 15:07:19.000-0600 I don't think this is a problem because the builtin will take precedense anyway. By: Tilghman Lesher (tilghman) 2005-01-10 15:08:00.000-0600 Variables that applications set are not treated specially -- i.e. they do not represent special pointers into channel structures or system states.  For example, TIMESTAMP represents the current time and is continually changing.  Something like SIP_INFO is simply a normal variable which is used by the SIP stack. By: Tilghman Lesher (tilghman) 2005-01-10 15:13:17.000-0600 Yes, the builtins take precedence, but we should warn the user when they attempt it -- it's valuable debugging advice that the user has happened to have chosen a reserved variable. By: Mark Spencer (markster) 2005-01-10 16:16:44.000-0600 I'm not willing to take the performance hit for this either.  I'm not worried about this, but it's perhaps worthwhile to have it in the bug tracker anyway. By: Tilghman Lesher (tilghman) 2005-01-10 16:20:40.000-0600 I don't see how this is a performance hit at all.  You still have to iterate through a list to find if a variable is already set and delete it before adding a new one.  All this does is prevent setting variables which cannot ever be referenced (saving both time and memory). By: Mark Spencer (markster) 2005-01-13 00:12:09.000-0600 It is a performane hit because we have to compare it against a list of variables that we otherwise would not compare it to and the end result in any case is only to print a warning that would be obvious to anyone testing the code anyway! Not to mention, if this did go in, we would have to remember to add every variable to both functions. By: Tilghman Lesher (tilghman) 2005-01-13 01:07:21.000-0600 Right, but when the specified special variable is changeable, then the patch changes the underlying channel structure, and Asterisk behaves as expected. By: Mark Spencer (markster) 2005-01-13 08:48:25.000-0600 I don't really think that sounds like a good thing either.  Not only is it not consistent (e.g. DNID, CONTEXT, etc) but your very title makes it clear these special variables are "read only".  My feeling is they should stay that way.  Ignoring writes is both easier to maintain and higher performance and given the presense of README.variables I do not believe presents a legitimate problem for the user. By: Tilghman Lesher (tilghman) 2005-01-13 09:10:09.000-0600 I don't see that as inconsistent.  Some variables are really read-only and some variables can be changed in the dialplan.  However, both of these types of variables refer to channel structures, not real variables, and thus should be referenced differently, both when read (as they are now) as well as when they are set (which this patch provides). By: Olle Johansson (oej) 2005-01-30 12:28:54.000-0600 Time to close this bug report? Or can we find a way to warn the user without performance hits? By: Tilghman Lesher (tilghman) 2005-01-30 14:15:43.000-0600 The two are mutually exclusive.  You cannot warn the user without detecting that an issue is present, and you cannot detect an issue without adding code.  I think this is an important enough issue, though, that we should handle it properly, which entails detecting the issue and adding the code. Note, however, that I've already optimized the code to have the least impact on performance, by reducing redundant comparisons.  I've done a similar thing when retrieving these special variables (in another bugset), which should also improve overall performance. By: Mark Spencer (markster) 2005-01-30 23:50:14.000-0600 It's definitely not worth the performance hit, but should explicitly be mentioned in the README.variables section.  Such checking could potentially be enabled if option_debug is set. By: Mark Spencer (markster) 2005-02-01 22:04:39.000-0600 I've updated the README.variables to explicitly describe this behavior. By: Russell Bryant (russell) 2005-02-06 21:38:01.000-0600 added to 1.0, as well By: Digium Subversion (svnbot) 2008-01-15 15:24:06.000-0600 Repository: asterisk Revision: 4951 U   trunk/doc/README.variables ------------------------------------------------------------------------ r4951 | markster | 2008-01-15 15:24:05 -0600 (Tue, 15 Jan 2008) | 2 lines Warn about setting read only variables (bug ASTERISK-3243) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4951 By: Digium Subversion (svnbot) 2008-01-15 15:24:33.000-0600 Repository: asterisk Revision: 4974 U   branches/v1-0/doc/README.variables ------------------------------------------------------------------------ r4974 | russell | 2008-01-15 15:24:32 -0600 (Tue, 15 Jan 2008) | 2 lines update readme (bug ASTERISK-3243) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4974