Summary:	ASTERISK-03216: [PATCH] implement new method of setting channel variables from config files
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2005-01-08 12:35:42.000-0600	Date Closed:	2011-06-07 14:10:19
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/Configuration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) sip_iax_queue_at_variables_rev1.diff.txt
Description:	(This patch actually touches chan_sip, chan_iax2 and app_queue, but it's configuration-related, so I put it into this category as it seemed most appropriate). The attached patch implements a new syntax for setting channel variables from sip.conf and iax.conf, and as a bonus adds the ability to set them from queues.conf as well. The new syntax is: [foo] type=user .... @var1=value1 @var2=value2 This would result in channel variables named "var1" and "var2", with the expected values. This syntax is meant to replace the "setvar=" syntax, although that is still supported (but it will generate a single deprecation warning the first time it is used). The advantages to this syntax are: - it plays well with the "defaults" patch in ASTERISK-3063099 - it is much easier for RealTime config users to use; instead of a single db column named "setvar" with multiple values in it, they can have columns named "@var1", "@var2", etc. with their desired values (some databases may not allow columns with leading @, so this syntax might need to change slightly) ****** ADDITIONAL INFORMATION ****** Disclaimer is on file.
Comments:	By: Brian West (bkw918) 2005-01-08 14:36:45.000-0600 we already ahve this btw setvar=VAR1=var in both sip and IAX. By: Brian West (bkw918) 2005-01-08 14:39:31.000-0600 hrm you're right on realtime you could use a ; as the seperator maybe? like the allow= line was till we fixed it where the allow could have them in a list. bkw By: twisted (twisted) 2005-01-08 14:45:11.000-0600 I believe this method will be fine - as long as we still retain backwards compatability, this would drastically improve the ability to set vars in a realtime environment. By: Olle Johansson (oej) 2005-01-08 14:58:18.000-0600 I do not like using the @ sign for this, it is already overloaded in a lot of various constructs in asterisk configuration files. Don't forget to make sure the construct is compatible with "_variables" and "__variables" We could use the macro-name construct, like setvar-custno=12345 setvar-_alert_info=signalcode By: Brian West (bkw918) 2005-01-08 15:04:06.000-0600 It already works with realtime .. check line 150 in res_config_odbc.c You'll see a while loop.. setvar with this as the value "VAR1=var1;VAR2=var2" That should alread work... and falls in line with how mark wanted it. (Since he wrote that) bkw By: twisted (twisted) 2005-01-08 15:05:24.000-0600 I'm not talking about from within asterisk... i'm talking about externally, editing/changing the database configuration 'file' from _OUTSIDE_ of asterisk. By: Kevin P. Fleming (kpfleming) 2005-01-08 17:02:01.000-0600 Yes, bkw, you are correct, you can already put multiple variable values into a single column in a realtime config. The problem is, this forces you to put _unrelated_ values into the same column (unlike allow, or mailbox, where multiple entries are actually multiple entries for the same setting). In other words, the effect of this patch on a realtime config is much the same as we have now with all the other columns... if we extended the logic you are proposing here we could just have a single column in the db containing: "type=peer;secret=foo;host=dynamic;nat=yes;qualify=5000" Since the code can split this apart, this would work. The reason we don't do this to people is that it's a maintenance nightmare; if you want to change the "nat" value, you have to split apart and reconstruct the entire string to update that column in the database, which can affect all the other unrelated values in that string. Using my technique allows for all unrelated values to be in their own columns. oej: I am not opposed to using some other flag character besides @, but I don't think that "setvar-foo=value" is very intuitive either... since we are setting variables that will later be referenced using $, how about that? [foo] type=user nat=yes ... $extension=123 $othervalue=456 By: Brian West (bkw918) 2005-01-08 17:09:49.000-0600 splitting and joining the values is a simple in most languages on the backend... in php its explode/implode in perl is split/join I see no problem with how it works now.  You can't just remove setvar. bkw By: Kevin P. Fleming (kpfleming) 2005-01-08 17:26:17.000-0600 Did you read the bug notes? I did not remove "setvar", I only deprecated it in favor of a simpler and more readable syntax. Also, since setvar has never been released in a stable release, it _can_ be removed, if Mark wishes to do so. Putting this patch in and then waiting for a couple of months would suffice; by that time anyone using CVS HEAD and "setvar" would have had plenty of time to see the warning in their logs and take corrective measures. By: Kevin P. Fleming (kpfleming) 2005-01-08 17:28:17.000-0600 Also, forgot to mention, whoever allowed ASTERISK-2852882 (the implementation of setvar) to be merged unchanged should take a second look... It added "#include config_pvt.h" to both chan_sip and chan_iax2, only so they could get access to ast_new_variable. My patch moves that prototype to config.h and stops chan_sip and chan_iax2 from including config_pvt.h, which is supposed to be private to config.c. By: Anthony Minessale (anthm) 2005-01-08 20:05:46.000-0600 Nobody has any issue with the configuration parsing rules being warped by this new construct that does not exist at all anywhere else? This is the prime reason i kept the syntax in line with the standard that has already existed when I made patch 2882 and made no attempt to dream up any new ones.   As silly as it may seem, sticking to the basics are very important in this kind of thing since a chatoic result can come from resyntaxing. Maybe you shold submit a patch to fix the prototyping concerns seperatly and try not to take a shot at me like that, If I didn't do the hard part aleady it would be a lot less possible, I chose to leverage the existing framework and build off of it and this mindset has proven the best way to go time after time no matter how small and petty it may seem, this issue of LHS (left hand side) and mutiple values was always apparent when this realtime extension was designed and was livable in light of being able to provide this useful abstraction layer.  Proponents of this patch will surely reply ripping on me although I hope I'm wrong.  I think there is a better more elegant solution right around the corner so you may want to ponder it a little more before proceeding. The opinions expressed in this bugnote are not necessarily those of this station. Some personalities that appear in this bug note have been compensated for thier endorsments. By: Anthony Minessale (anthm) 2005-01-08 20:54:06.000-0600 Hey how bout this...? What if the config parser strips all leading 0-9 and _ from any varname as it builds the config. then in the db you can have _setvar __setvar 01_setvar 02_setvar and they are unique for the db's sake and all rewritten as "setvar" as far as asterisk is concerned. This would fix the problem for every duplicated varname issue like allow, disallow etc not just setvar. I'm assuming no varnames start with numbers of corse but you could always use some other convention with the same idea like _a_setvar if first char is _ ignore all chars till you see another _ Just a brainstorm fell free to engulf it in flames. By: Kevin P. Fleming (kpfleming) 2005-01-08 21:07:41.000-0600 Well, I already use the ability to create a channel variable called __CALLSOURCE from my sip.conf, so that I know that variable will be carried through all channels created by any operations done by that channel (since the double-underscore means inherit to all sub-channels). That means stripping underscores wouldn't work. I'll have to let someone else chime in on the idea of just using numeric prefixes; I think that would work (there are no variables currently that start with digits that I can find), but it's odd to make the user use arbitrary numbers that don't have any meaning associated with them. By: Anthony Minessale (anthm) 2005-01-08 21:18:40.000-0600 How bout just consider anything that is not significant to be a var name blah => yes blah is not a vaild option so it is automaticly a var again more brainstorming. By: Kevin P. Fleming (kpfleming) 2005-01-08 21:52:47.000-0600 I had originally done that, then I mistyped "type=friend" in sip.conf and it broke, because chan_sip accepted it anyway and there was no "type" setting for that category. I think it needs to be some specific indicator that these variables are intended to be put into the channel for use in the dialplan; that also keeps us from adding a new chan_sip variable in the future and breaking someone's existing config that happened to use that variable for their dialplan. By: Brian West (bkw918) 2005-01-08 21:56:31.000-0600 Why not take everything thats uppercase? VARNAME=blah you do have a way to tell if its upper case right? bkw By: Kevin P. Fleming (kpfleming) 2005-01-08 22:03:11.000-0600 Sure, we could do that, as long as someone mandates that channels/apps/modules can never use uppercase variable names for their own purposes. Also, I don't know if the realtime db drivers will differentiate between case in column names. That's what concerns me: it's not obvious what is happening unless we make the user specifically do something. Can someone explain what the big difference between setvar=SOURCE=foo and @SOURCE=foo or $SOURCE=foo is? As best I can tell, it's only a simple text change, with the same syntax and semantics after the 'indicator'. By: Olle Johansson (oej) 2005-01-09 03:20:05.000-0600 kpfleming: Well, I would hate to see the "select $skrep from" in my perl or php source... Would not enhance readability... edited on: 01-09-05 03:20 By: Olle Johansson (oej) 2005-01-09 03:24:56.000-0600 setvar= is easier to handle within the current source code, it's only one config label with a value that we are handling... I do not like the double equal signs though. ...and I can't come up with a good proposal. We need something that works both in SQL, programming languages and have some similarity with current asterisk configurations - or at least does not break the current syntax, but enhance it. maybe just a "var_" prefix... var_skrep=tuss By: Kevin P. Fleming (kpfleming) 2005-01-09 07:30:57.000-0600 Then I'll propose using "chanvar_" as the prefix, which is pretty clear and specific what it's purpose is. The modules would then strip that prefix, but any subsequent underscores, so that still allows you to create channel variables with leading underscores in their names. The patch will continue to allow "setvar=name=value" with a deprecation warning, but will prefer "chanvar_name=value"? Everyone OK with that? By: Anthony Minessale (anthm) 2005-01-09 08:58:47.000-0600 Just FYI Don't forget you can have: setvar => var=val if double equal makes you uneasy =D would chanvar_blah => true yield ${blah} then? I could see supporting this for the sake of sql realtime but maybe not depreciate the regular way just accept both because file configuration users never need to bother learning the syntax vioiation it proposes.  In reality the new way is the one that should be scheduled for depreciation.  What bugs me is that this only fixes 1 issue and not the fundamental one i.e. mutiple occuracnes of the same left hand operand in a databse system that requires unique col names. By: Kevin P. Fleming (kpfleming) 2005-01-09 09:09:57.000-0600 I don't understand... chanvar_blah => true would result in a channel variable called "blah" with a value of "true". Please explain how this is a syntax "violation". In fact, it _exactly_ solves the problem you are referring to, of creating unique variable names (left-side values) so that databases can deal with this stuff more easily. By: Anthony Minessale (anthm) 2005-01-09 09:32:48.000-0600 sigh, It only solves 1 case not all of them.  I said it would maybe be ok to use it for the sake of realtime but it is indeed a syntax violation because you are not using a distinct lhs => rhs you are making a dynamic lhs and trimming off which is an exception to the fundamentals. We are talking about the core CVS asterisk here so we must always be careful what we decide to accept as a permenant convention with very much consideration. I'd just hate to see this fix the more popular variety of this problem and forget about all the other ones once it's adoptend and it can also lead to a mutiny of configuration rules.  I guess I'd better get my flame retardant underwear out. cos I don't see anyone agreeing with me. By: Olle Johansson (oej) 2005-01-09 09:38:48.000-0600 Kind of agree with anthm, it would be great if we could find a generic solution. The stuff we currently have in the text based version of voicemail.conf drives me mad. It is against all config syntax we have. Anthm: I like setvar => var=value The => is defined as "create an object" in zapata.conf and the original handbook. Even though that doesn't solve this problem. By: Kevin P. Fleming (kpfleming) 2005-01-09 09:55:06.000-0600 anthm: I can't agree with you yet because you have not yet told me what these "other" db-related problems are that the patch doesn't solve. I'll be happy to address them when I know what they are :-) As far as a "dynamic" LHS, this patch is no more dynamic than the existing syntax. Currently: setvar=foo=value (or) setvar => foo=value Neither of these result in any object called "setvar" being created at all, ever. They only result in a variable called "foo" being put into a list to be used later, so if you want to talk about what objects are being created, they create an object named "foo". New: chanvar_foo=value (or) chanvar_foo => value Again, this does not result in an object called "chanvar_foo" being created. It results in an object called "foo" being created, the same as the old syntax did. In other words, we _already_ have syntax that "violates the rules", because it appears to be creating an object called "setvar" but it does not do that. If we are really concerned about not violating the rules, then we need to change that as well. By: Anthony Minessale (anthm) 2005-01-09 10:20:20.000-0600 take a step back. setvar is 1 option that is only valid for sip and iax I'm talking about the overall concept if you must have examples. allow disallow come to mind you can have many of these I'm just asking if we can find a solution that solves the problem forever not just 1 instance of it. Don't forget this is only a problem for rdbms, ldap for instance supports mutiple lhs we can't be biased towards any 1 system when we are trying to be abstract.  I have a few config engines that are not using a db at all and do not have this issue. I'ts not like I am the one that is going to adopt it into CVS, ask markster it's his decision. He carefully governs configuration directives I'm just trying to help you get your patch adopted by telling you what I think it needs to succeed. I can see pride is gonna start getting in the way here so I am not gonna comment anymore on this issue 1 final time I will say I can live with the chanvar_blah thing but not as the official way rather an alrernate way that is only advertised to realtime users. By: Kevin P. Fleming (kpfleming) 2005-01-09 10:52:40.000-0600 I think you are missing the point here Tony. I'm trying to explain it as best I can, but apparently it's not working :-) I don't see allow/disallow as a related issue at all, for two reasons: - multiple allow/disallow are _related_ to each other - allow/disallow already support multiple values in a single line (thanks to your recent work) The reason that I don't think this is related is because multiple setvar (or chanvar_) lines are _not related to each other_ at all. If I put chanvar_extension=320 chanvar_externalid=kevin chanvar_queue=no These entries do not have anything to do with each other, so it's logical that their left-hand values (object names, labels, whatever) are different. Pride has nothing to do with this; I just want a solution that produces unique left-side names for entries that are not related to each other, and that does not negatively impact current and future syntax of configuration files. This really has nothing to do with realtime (I don't even use it), but I pointed out that it would help some realtime users who currently have to bundle multiple setvar entries into a single db column, even though those entries do not belong together (unlike multiple allow/disallow entries, which do belong together). I have received a number of positive comments on this patch via IRC and the conference call... I wish some of those users would weigh in with their opinions here, so I can learn if I am just going in the wrong direction or if the current "issues" are not really issues. By: Anthony Minessale (anthm) 2005-01-09 12:21:54.000-0600 wtf? all the solutions discussed here including my few suggestions are kludges it is indeed pride or you would not continue to argue after i said it was acceptable 3 times now I just refuse to condone making it the "official" way and generating deprecation messages.  Having people love your patch is not enough btw, I have had tons of patches that never have a hope of making CVS that people applaud me for daily so get used to it. By: Anthony Minessale (anthm) 2005-01-09 12:23:26.000-0600 Reminder sent to markster Please comment on this bug. By: Mark Spencer (markster) 2005-01-16 02:18:09.000-0600 I don't especially enjoy any of the proposed solutions or to some degree even what is there now, but I have nothing better to add or suggest, and i do recognize the value in being able to have these "predefined" channel variables.   Until something comes along that *really* provides additional advantages as compared with the current "setvar=" syntax, I see no reason to change it.  There's my $0.02 since you asked :) By: Kevin P. Fleming (kpfleming) 2005-01-16 08:54:42.000-0600 Well, to go back to my original comments in this bug, I think this _does_ provide additional value: it allows for predefined channel variables to be used in default categories (see ASTERISK-3063099), and it allows realtime-config users to split their predefined channel variables up into separate database columns. To make that work well will probably require one of the versions that doesn't use special punctuation (chanvar_foo would likely be the best candidate), as not all database engines can handle column names like that. By: ckruetze (ckruetze) 2005-01-16 10:58:00.000-0600 To allow realtime-config users to split channel variables up into seperate database columns, you don't need to change Asterisk. All you need is a database that supports views, stored procedures or something like that. Nowadays most of them do. You could dynamicly build your config files as complicated as you want, just by using the right SQL statements without changing code inside Asterisk. By: Brian West (bkw918) 2005-01-16 11:01:19.000-0600 ckruetze BINGO!!! bkw By: Kevin P. Fleming (kpfleming) 2005-01-16 11:07:41.000-0600 So you feel that it's more efficient to take multiple columns and combine them together, only so that Asterisk can split them back apart again? That's not what I would have expected :-) Also, I'd like to see an SQL view that combines multiple columns together and puts proper punctuation between the entries... that is tricky to write, and certainly not something we should force every future realtime user to have to do. By: Mark Spencer (markster) 2005-01-16 11:14:39.000-0600 You can already use setvar multiple times and thus it should work fine with inherited categories (I assume this is what you mean by "default" categories).  I  remain unconvinced that the overhead of splitting on ";" is by itself sufficient to merit this change in syntax. By: Kevin P. Fleming (kpfleming) 2005-01-16 11:29:31.000-0600 Here is the problem with default categories: [defaults] ... setvar=foo=value1 setvar=foo2=value2 [peer](defaults) setvar=foo=value9 If you do this, all the "setvar" lines from the defaults category will be lost, because when you specify a value for any particular variable in the "real" category, it _must_ replace the value that came from the defaults category. Since all the "setvar" lines have the same variable name, they all get removed and replaced with the new (single) value. If you instead use: [defaults] setvar=foo=value1;foo2=value2 [peer](defaults) setvar=foo=value9 You have the same problem; you cannot replace any single value provided in the setvar "defaults" without replacing them all. Since they are not multiple values for the same variable, but are in fact unrelated entries, combining them into a single line (like allow=/disallow=) is not logical in my opinion. If people feel so strongly that the current syntax is the best way to do this, then I'll be happy to retract this patch and keep it in my local trees... it certainly makes configuring our systems tremendously easier. By: Mark Spencer (markster) 2005-01-16 16:27:11.000-0600 In your first example, you would import both lines from the [defaults] context, and then add the third as specified, resulting in the equivalent of: [peer] ... setvar=foo=value1 setvar=foo2=value2 setvar=foo=value9 Which would effectively be equivalent to: setvar=foo2=value2 setvar=foo=value9 What am I missing? By: Kevin P. Fleming (kpfleming) 2005-01-16 16:57:12.000-0600 The "defaults" patch currently replaces any existing variables in the context (that came from the defaults) with the ones specified directly in the current context. I did that because I didn't think it was safe to rely on every module always being able to handle the same variable being specified more than once (and only accepting the last specified value). This is pretty much mandatory, as if you set "context" in the defaults, and you want to replace it in the user's entry, you cannot do so if the channel module supports two (or more) contexts for the user (as chan_iax2 currently does). In fact, as it stands right now, if you use "setvar=foo=value" twice in sip.conf or iax.conf, the channel module will create two variables with the same name in its internal structures, and then pbx_builtin_setvar_helper will eliminate the duplicate when the channel is created. In summary, I felt it was most logical and understandable if "default" values were completely replaced by values specified in the final context in the "chain"... but this only works if the variable names are as unique as they can be. By: Mark Spencer (markster) 2005-01-30 01:29:16.000-0600 After consulting with kpfleming we've agreed that, having merged inheritance, this  functionality exists within current config syntax.