Summary:	ASTERISK-03042: [PATCH] config category inheritance
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2004-12-19 11:49:25.000-0600	Date Closed:	2008-01-15 15:23:42.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/Configuration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) config_addition_inheritance_rev3.diff.txt
Description:	This patch allows for "inheritance" of variables from one config category (context) to another. It also support inheriting from multiple contexts, and inheritance chains (defaults -> foo -> bar). Example: File: /etc/sip.conf [defaults] type=friend host=dynamic auth=md5 qualify=5000 #include a/sip.conf File: a/sip.conf [defaults-a(defaults)] context=a#sip-phones accountcode=a secret=our-fancy-default [peer-1(defaults-a)] callerid=Peer 1 <301> mailbox=301@a [peer-2(defaults-a)] callerid=Peer 2 <302> mailbox=302@a auth=plain This method works for all config files, although it is not usable in voicemail.conf because of the structure there, and it is not really useful in extensions.conf (although it would work fine). Where it is most useful is sip.conf, iax.conf, queues.conf, dundi.conf, iax-prov.conf, manager.conf, mgcp.conf, rpt.conf and skinny.conf. In fact, with this patch, all of these gain a single, common, understandable way to set defaults at a global and/or file level, without requiring any modifications to the modules that use them. There is one known restriction: you can only inherit a single value with any given name from a "base" category; if that category has three entries for "foo", only the last one will end up being inherited, the others will be ignored. The patch is relative to the patches in bugs ASTERISK-3203237 and ASTERISK-2993020. Also, once this patch is merged (assuming it is), it would be possible to start deprecating all the globals support in sip.conf, iax.conf and etc in favor of using this technique; in all of those files/modules, any variables set at the global level that have no "global" effect but are just defaults for later entries in the files could be moved to a "defaults" category, and the code required to support them removed, since the config parser can now handle it completely. ****** ADDITIONAL INFORMATION ****** Disclaimer is on file.
Comments:	By: Olle Johansson (oej) 2004-12-19 13:12:05.000-0600 For sip, I see a problem where people need different defaults for users and peers... By: Kevin P. Fleming (kpfleming) 2004-12-19 13:43:27.000-0600 Well, I'll agree with that, but that is not functionality that we currently have, so this patch does not take it away :-) I would suggest that if the user has that need, then the variables that differ between users and peers can be handled one of two ways: - don't put them in the defaults, and repeat them in each user/peer - put all the users into one file, and all the peers into another file, include both files from the main file, and set the defaults appropriately in each sub-file; if there are defaults in common between the users and peers, a 3-level include tree can be used to accomplish this By: Kevin P. Fleming (kpfleming) 2004-12-19 14:17:36.000-0600 Actually, there is a third alternative: the "defaults" category can be specified multiple times in the same file, and will override (on a variable-by-variable basis) previously specified defaults in that file. So, for the user/peer case, you could have one defaults category at the top of the file that applied to all the peers, then all the peers, then another defaults category with only the changes that applied to users, then all the users. By: Olle Johansson (oej) 2004-12-19 14:22:10.000-0600 Seems like this was no problem, I rest my case :-) Good architecture! By: Mark Spencer (markster) 2005-01-01 14:58:26.000-0600 I prefer keeping the text parsing as simple as possible so that other people can write their own parsers too.  I especially don't want anything that could suffer any performance loss at all.  So basically, I'm going to need a lot more convincing on this one :) By: nick (nick) 2005-01-01 16:19:25.000-0600 I can see definite security utility in this patch, especially for companies with branch offices or ASPs hosting Asterisk for multiple companies.  With this patch, permissions could be set such that the IT person for each branch office/company could only update their own particular realm without being able to significantly affect others.  At least, that's the advantage I see :-) By: Kevin P. Fleming (kpfleming) 2005-01-01 16:33:52.000-0600 I hadn't thought of nick's point before, but that's a good one. To address the "complexity" issue, I'd suggest taking a hard look at the patch :-) I'd estimate that 60% of it is not really related to "defaults" but actually is refactoring of the existing code to make it more obvious and/or more efficient. The code related to storing and applying defaults is likely less than 100 lines, although I haven't counted it directly. Also keep in mind that if we choose to do so, we can eliminate hundreds of lines of code from chan_sip, chan_iax2, app_queue, etc. that all do their own "global" options parsing now, but would no longer need that. Granted, that's a non-backwards-compatible change, and I don't know what the policies are in that area (since there has only been one "stable" release so far), but from a code maintenance and documentation standpoint it'd be a big win in my opinion. By: Mark Spencer (markster) 2005-01-01 16:58:43.000-0600 yah but basically, really, we're talking about nothing that can't already be done with #includes, the only difference is it's done automatically without your saying "#include foo.conf" By: Kevin P. Fleming (kpfleming) 2005-01-01 17:13:10.000-0600 Uhh... no. I think you've misunderstood what this patch does. There is no way to accomplish this patch's functionality with Asterisk's current config parser. Each config file currently has _one_ set of global defaults that apply to all contexts in that file (and any files it includes), and that set may or may not support all the variables that can be set in those contexts... it depends on the implementation in the module that reads the config. For example, there are lots of settings in queues.conf that cannot be set at the global level, because app_queue does not have support for "global" versions of those settings. What this patch does is completely remove the need for the config-using module to even have to implement "global" settings at all, unless those settings are truly global for the module, and not just applied to all its contexts. For example, "persistentmembers" would continue to be a global setting for app_queue, but this patch allows for "strategy", "maxlen", "retry", etc. to also be global, without adding a single line of code to app_queue. In addition to that, these defaults can be overridden on a file-by-file basis, so if you structure your config files (using includes) by customer, for example, it's easy to make defaults that apply to everyone, then customer-specific defaults when you need them. A real-world example is this: Our top-level sip.conf has defaults for these variables: type, host, nat, auth, dtmfmode, qualify, disallow and allow. Note that some of these are _not_ available as global defaults in chan_sip normally, and I did not add any code to chan_sip to support them. Our customer-level sip.conf has defaults for these variables: secret, context and accountcode (along with some local variables). Again, none of these are settable at the "global" level in chan_sip, and even if they were they would apply to all my customers, which would not work. This results in entries inside the customer-level sip.conf file needing only two variables to be completely defined: callerid and mailbox. Does that make it clearer what this patch accomplishes? By: Kevin P. Fleming (kpfleming) 2005-01-01 17:16:05.000-0600 Well, I guess I spoke a little too soon... sure, you could have something like: [foo.bar] #include defaults.sip.conf #include customer2.sip.conf callerid=... mailbox... in the customer-level sip.conf file, and that would accomplish the desired result, sort of. It's not very pretty, and relies on variables overriding each other (for variables that can be specified multiple times, this won't work). It's also tremendously more work for the config parser to process, since it has to repeatedly open and parse these files for every single context in the files that use them. It also takes the defaults out of this customer-specific file, doubling the number of files that need to be maintained. By: Mark Spencer (markster) 2005-01-01 19:11:44.000-0600 To be reimplemented as a(b,c,d)... By: Mark Spencer (markster) 2005-01-04 00:02:09.000-0600 Does this do the right thing with [foo(foo)] ? By: Mark Spencer (markster) 2005-01-04 00:02:43.000-0600 Also what happens if i do: [foo] [bar(foo)] [foo(bar)] ? By: Kevin P. Fleming (kpfleming) 2005-01-04 09:46:31.000-0600 Hadn't yet thought about those bizarre cases... let's see: Case 1: [foo(foo)] This will have no effect, as at the time that inheritance is processed, the "foo" context has been added to the config, but it is empty. inherit_category will then scan through the context and find no variables, and return. Case 2: [foo] [bar(foo)] [foo(bar)] This will actually work just fine :-) "bar" will contain everything from the first "foo", plus its own variables. The second "foo" will contain everything from "bar", which already contains everything from the first "foo". In that sense, my note in the description about not supporting chains is wrong; I will correct it. However, I have realized there is one flaw in this implementation, compared to my initial one: the categories created _only_ to supply defaults to be inherited by other categories are processed as "normal" contexts by the module using the config. This is bad, because I end up with incomplete SIP peers named "defaults", "def-client1", "def-client2", etc. With the original implementation, the "defaults" contexts were never visible to the module using the config. Somehow we need a way in the config parser to "hide" these categories from the module using the config. By: Mark Spencer (markster) 2005-01-09 04:02:46.000-0600 One option would be that [(foo)] will only be considered for inclusion and never shows up in category_walk or category_retrieve. By: Kevin P. Fleming (kpfleming) 2005-01-09 07:36:38.000-0600 Hmm... that's an unusual derivation of the syntax, but it would work. I have another suggestion, based on the new rev of ASTERISK-2993020 (I don't know if you've had time to look at that one or not...) That patch adds a new type of directive to config files, "#parser". This could be used as follows: [defaults] ... ... ... #parser hide-category defaults #parser default-categories defaults This would accomplish both purposes; it would hide "defaults" from all category browsing/searching done by the module, and it would apply the "defaults" category to all further categories (until it was changed, or it was set to "none"). Thoughts? By: Kevin P. Fleming (kpfleming) 2005-01-09 09:12:21.000-0600 New rev, updated to apply on top of current patch in ASTERISK-2993020. By: Kevin P. Fleming (kpfleming) 2005-01-15 20:10:05.000-0600 Rediffed against current CVS and patch in ASTERISK-2993020, plus moved functions needed by this bug into this patch from ASTERISK-3203237. By: Mark Spencer (markster) 2005-01-16 01:13:29.000-0600 I'm wondering if we should be placing these syntactic things outside of the []'s just in case people used any special characters within the braces. [foo](bar,baz) ([foo]) The whole "#parser" thing still bothers me (in fact, all this syntactic sugar in config.c is pushing the limits of what my intention of what the capabilities of static configuration are). By: Mark Spencer (markster) 2005-01-16 01:31:28.000-0600 Perhaps (ignored) would be good, e.g.: [foo](ignored) [bar](foo,ignored) [baz](bar) By: Kevin P. Fleming (kpfleming) 2005-01-16 08:50:44.000-0600 I could live with your last proposal, but it has the downside that it assigns a "magic" category name again (ignored). However, in this case, the user can still name a category "ignored", they just can't use it as a source of defaults. That seems tolerable :-) By: Mark Spencer (markster) 2005-01-16 09:01:49.000-0600 I agree that the "magic category name" of "ignored" is a downside (open to other suggestions), but at least by always having things in () afterwards keeps the "fast path" fast, that is that if no one is using this stuff, the *only* check will be for the presense of '(' after the check for ']'. By: Kevin P. Fleming (kpfleming) 2005-01-16 09:34:00.000-0600 The "fast path" already works that way; once it has determined that the current line contains a properly-constructed category header (opening [ and closing ]), then it checks once to see if there is a ( inside the category name. If not, nothing else gets done differently than currently happens. Checking for ( outside of ] would not be any faster or slower :-) Since this patch is no longer dependent on ASTERISK-2993020, I'll post a new version with your suggested change and independent from it. By: Mark Spencer (markster) 2005-01-16 10:05:10.000-0600 Checking for ( within the category name would require strchr whereas checking for ( after the ] is just one operation since it can only be immediately following it.  It may be possible to syntactically unify this patch with the append patch with something like: [foo](bar,+) where the + signifies that it's supposed to append to an existing "foo".  Then *all* this extra functionalitiy could hide between one solitary if (c[1] == '(') { } If you don't like the reserved word "ignored" then we could use "-" to signify ignored, or we can make all of the terms begin with a reserved symbol or phrase, e.g.: [foo](<=bar,<=baz,append,ignored) Thoughts? By: Kevin P. Fleming (kpfleming) 2005-01-16 10:13:45.000-0600 You are correct, as I've been working on the new version I realized that the direct check for '(' is faster. I like your thought about using a flag character for this function, but I think that '!' would be more appropriate (since people already are used to it being "not", well, at least us programmers are <G>). So, then the syntax would be: [name]([+,][!,][name,...]name) (although they would not need to be specified in that order) By: Kevin P. Fleming (kpfleming) 2005-01-16 13:19:52.000-0600 OK, new version, combines functionality from ASTERISK-2993020 into this patch. Syntax is as posted in the previous bugnote. By: Mark Spencer (markster) 2005-01-16 16:40:17.000-0600 Why are we keeping track of whether a variable is inherited? By: Kevin P. Fleming (kpfleming) 2005-01-16 16:58:06.000-0600 So we know whether to replace it or not with later specified values (see my last bugnote in ASTERISK-3243278). By: Mark Spencer (markster) 2005-01-16 17:20:27.000-0600 I believe that the context in IAX is the *only* such example, so it would be easier just to have something like "context=" clears out the list. By: Mark Spencer (markster) 2005-01-20 22:36:31.000-0600 I think the "inherited" stuff should probably go away for consistency and we'll just fix up the things that are otherwise broken. By: Kevin P. Fleming (kpfleming) 2005-01-22 13:09:31.000-0600 New version uploaded, inheritance flag is gone, and new behavior (supplying empty value erases previously supplied values) implemented. Just for the record, this multiple-value problem also occurs with allow/disallow and permit/deny... all of those can be specified multiple times and add together, rather than replacing each other. However, this new replacement syntax is adequate to express what is needed: if one (or more) of the categories you are inheriting from have specified multiple "allow=" settings, you can override them all by using: [peer](defaults) allow= allow=ulaw Works for me :-) By: Kevin P. Fleming (kpfleming) 2005-01-29 17:53:15.000-0600 New rev, no functionality changes, just code improvements and update to apply to CVS HEAD (post-ASTERISK-3373406). By: Mark Spencer (markster) 2005-01-30 00:24:44.000-0600 Added to CVS (sans changes to stock configs) with documentation promised to follow. By: Russell Bryant (russell) 2005-01-30 23:25:26.000-0600 added to 1.0 ... just kidding :) By: Digium Subversion (svnbot) 2008-01-15 15:23:42.000-0600 Repository: asterisk Revision: 4925 U   trunk/config.c ------------------------------------------------------------------------ r4925 | markster | 2008-01-15 15:23:42 -0600 (Tue, 15 Jan 2008) | 2 lines Add category inheritance (bug ASTERISK-3042) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4925